Skip to content

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

Merged
roryqi merged 1 commit into
apache:masterfrom
rickyma:issue-1189
Jan 15, 2024
Merged

[#1189][FOLLOWUP] fix(server): Start NettyDirectMemoryTracker.#1432
roryqi merged 1 commit into
apache:masterfrom
rickyma:issue-1189

Conversation

@rickyma

@rickyma rickyma commented Jan 10, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

rickyma commented Jan 10, 2024

Copy link
Copy Markdown
Contributor Author

PTAL @jerqi @zuston @lifeSo

@roryqi
roryqi requested a review from lifeSo January 10, 2024 15:53
@roryqi

roryqi commented Jan 11, 2024

Copy link
Copy Markdown
Contributor

@lifeSo Could you help review this pr?

@zuston

zuston commented Jan 12, 2024

Copy link
Copy Markdown
Member

@lifeSo Could you help review this pr?

ping @lifeSo

@lifeSo

lifeSo commented Jan 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

@lifeSo Could you help review this pr?

ping @lifeSo

I reviewed yesterday, I try another way.

image

@roryqi

roryqi commented Jan 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

@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

lifeSo commented Jan 12, 2024

Copy link
Copy Markdown
Contributor

@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

lifeSo commented Jan 12, 2024

Copy link
Copy Markdown
Contributor

@rickyma

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

@lifeSo

lifeSo commented Jan 12, 2024

Copy link
Copy Markdown
Contributor

@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

@lifeSo lifeSo left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread 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;

@lifeSo lifeSo Jan 11, 2024

Copy link
Copy Markdown
Contributor

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'

@lifeSo lifeSo Jan 12, 2024

Copy link
Copy Markdown
Contributor

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

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

registerHeartBeat.startHeartBeat();
directMemoryUsageReporter.start();

@lifeSo lifeSo Jan 11, 2024

Copy link
Copy Markdown
Contributor

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.

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

registerHeartBeat.startHeartBeat();
directMemoryUsageReporter.start();

@lifeSo lifeSo Jan 12, 2024

Copy link
Copy Markdown
Contributor

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

@lifeSo lifeSo left a comment

Copy link
Copy Markdown
Contributor

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

rickyma commented Jan 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

rickyma commented Jan 12, 2024

Copy link
Copy Markdown
Contributor Author

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

lifeSo commented Jan 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

rickyma commented Jan 13, 2024

Copy link
Copy Markdown
Contributor Author

@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

@lifeSo lifeSo left a comment

Copy link
Copy Markdown
Contributor

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

roryqi
roryqi approved these changes Jan 15, 2024
View reviewed changes

@roryqi roryqi left a comment

Copy link
Copy Markdown
Contributor

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

@roryqi
roryqi 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

@roryqi roryqi roryqi 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 @roryqi @zuston @lifeSo