Skip to content

[#1189][FOLLOWUP] fix(server): Start NettyDirectMemoryTracker. #1432

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
jerqi merged 1 commit into from
Jan 15, 2024
Merged

[#1189][FOLLOWUP] fix(server): Start NettyDirectMemoryTracker. #1432

jerqi merged 1 commit into from
Jan 15, 2024

Conversation

@rickyma
Copy link
Contributor

@rickyma rickyma commented Jan 10, 2024 •
edited
Loading

What changes were proposed in this pull request?

Fix PR #1363.
NettyDirectMemoryTracker will not be started currently.

Why are the changes needed?

It's a followup PR for #1363.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing UTs.

@rickyma
Copy link
Contributor Author

rickyma commented Jan 10, 2024

PTAL @jerqi @zuston @lifeSo

@jerqi jerqi requested a review from lifeSo January 10, 2024 15:53
@jerqi
Copy link
Contributor

jerqi commented Jan 11, 2024

@lifeSo Could you help review this pr?

@zuston
Copy link
Member

zuston commented Jan 12, 2024

@lifeSo Could you help review this pr?

ping @lifeSo

@lifeSo
Copy link
Collaborator

lifeSo commented Jan 12, 2024 •
edited
Loading

@lifeSo Could you help review this pr?

ping @lifeSo

I reviewed yesterday, I try another way.

image

@jerqi
Copy link
Contributor

jerqi commented Jan 12, 2024 •
edited
Loading

@lifeSo Could you help review this pr?

ping @lifeSo

I reviewed yesterday, I try another way.

image

You should submit the review information, otherwise others can't see it.

@lifeSo
Copy link
Collaborator

lifeSo commented Jan 12, 2024

@lifeSo Could you help review this pr?

ping @lifeSo

OK， I reviewed yesterday as screenshot above. but I do not I should submit.
I re-reviewed and submitted comment.

@lifeSo
Copy link
Collaborator

lifeSo commented Jan 12, 2024

@rickyma

All is ok, but could add unit test case to test the tracker ?

@lifeSo
Copy link
Collaborator

lifeSo commented Jan 12, 2024

@rickyma

What is the difference between the two PlatformDependent?
Or is there problem using 'io.grpc.netty.shaded.io.netty.util.internal.PlatformDependent'

lifeSo
lifeSo reviewed Jan 12, 2024
View reviewed changes
Copy link
Collaborator

@lifeSo lifeSo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between the two PlatformDependent?
Or is there problem using 'io.grpc.netty.shaded.io.netty.util.internal.PlatformDependent'

@rickyma

server/src/main/java/org/apache/uniffle/server/NettyDirectMemoryTracker.java
import java.util.concurrent.TimeUnit;

import io.grpc.netty.shaded.io.netty.util.internal.PlatformDependent;
import io.netty.util.internal.PlatformDependent;
Copy link
Collaborator

@lifeSo lifeSo Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between the two PlatformDependent?
Or is there problem using 'io.grpc.netty.shaded.io.netty.util.internal.PlatformDependent'

Copy link
Collaborator

@lifeSo lifeSo Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between the two PlatformDependent?
Or is there problem using 'io.grpc.netty.shaded.io.netty.util.internal.PlatformDependent'

@rickyma

server/src/main/java/org/apache/uniffle/server/ShuffleServer.java
initMetricsReporter();

registerHeartBeat.startHeartBeat();
directMemoryUsageReporter.start();
Copy link
Collaborator

@lifeSo lifeSo Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add test unit case to test the funcation of directMemoryUsageReporter?
Or I could try later.

server/src/main/java/org/apache/uniffle/server/ShuffleServer.java
initMetricsReporter();

registerHeartBeat.startHeartBeat();
directMemoryUsageReporter.start();
Copy link
Collaborator

@lifeSo lifeSo Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add test unit case to test the funcation of directMemoryUsageReporter?
Or I could try later.
@rickyma

lifeSo
lifeSo reviewed Jan 12, 2024
View reviewed changes
Copy link
Collaborator

@lifeSo lifeSo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add test unit case to test the funcation of directMemoryUsageReporter?
Or I could try later.

@rickyma

@rickyma
Copy link
Contributor Author

rickyma commented Jan 12, 2024 •
edited
Loading

What is the difference between the two PlatformDependent? Or is there problem using 'io.grpc.netty.shaded.io.netty.util.internal.PlatformDependent'

@rickyma

When using Netty in the code, we should always use the io.netty:netty-all dependency instead of the io.grpc:grpc-netty-shaded dependency, because the io.netty:netty-all dependency is controlled by our project code (<netty.version>4.1.68.Final</netty.version>), while the Netty used by io.grpc:grpc-netty-shaded is controlled by gRPC's internal Netty version, which is uncontrollable.

@lifeSo

@rickyma
Copy link
Contributor Author

rickyma commented Jan 12, 2024

Could add test unit case to test the funcation of directMemoryUsageReporter? Or I could try later.

@rickyma

Please add the unit tests, thank you.

@rickyma rickyma requested a review from lifeSo January 12, 2024 06:59
@lifeSo
Copy link
Collaborator

lifeSo commented Jan 12, 2024 •
edited
Loading

What is the difference between the two PlatformDependent? Or is there problem using 'io.grpc.netty.shaded.io.netty.util.internal.PlatformDependent'
@rickyma

When using Netty in the code, we should always use the io.netty:netty-all dependency instead of the io.grpc:grpc-netty-shaded dependency, because the io.netty:netty-all dependency is controlled by our project code (<netty.version>4.1.68.Final</netty.version>), while the Netty used by io.grpc:grpc-netty-shaded is controlled by gRPC's internal Netty version, which is uncontrollable.

@lifeSo

OK, get it.

Could add test unit case to test the funcation of directMemoryUsageReporter?
Or I could try later.

@rickyma
Copy link
Contributor Author

rickyma commented Jan 13, 2024

@lifeSo Can you approve this PR? You can add UTs later. This PR is only for starting NettyDirectMemoryTracker.

lifeSo
lifeSo approved these changes Jan 13, 2024
View reviewed changes
Copy link
Collaborator

@lifeSo lifeSo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit is great and i think no problem.

If add test case in future is ok. @jerqi

jerqi
jerqi approved these changes Jan 15, 2024
View reviewed changes
Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, merged to master. Thanks @rickyma @lifeSo

@jerqi jerqi merged commit ad9fddd into apache:master Jan 15, 2024
@rickyma rickyma deleted the issue-1189 branch May 5, 2024 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jerqi jerqi jerqi approved these changes

+1 more reviewer

@lifeSo lifeSo lifeSo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rickyma @jerqi @zuston @lifeSo