You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "rickyma (via GitHub)" <gi...@apache.org> on 2024/01/10 08:52:19 UTC
[PR] [#1189][FOLLOWUP] fix(server): Start NettyDirectMemoryTracker. [incubator-uniffle]
rickyma opened a new pull request, #1432:
URL: https://github.com/apache/incubator-uniffle/pull/1432
### What changes were proposed in this pull request?
Fix PR [#1363](https://github.com/apache/incubator-uniffle/pull/1363).
### Why are the changes needed?
For PR [#1363](https://github.com/apache/incubator-uniffle/pull/1363).
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing UTs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1189][FOLLOWUP] fix(server): Start NettyDirectMemoryTracker. [incubator-uniffle]
Posted by "lifeSo (via GitHub)" <gi...@apache.org>.
lifeSo commented on code in PR #1432:
URL: https://github.com/apache/incubator-uniffle/pull/1432#discussion_r1449876841
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServer.java:
##########
@@ -142,6 +142,7 @@ public void start() throws Exception {
initMetricsReporter();
registerHeartBeat.startHeartBeat();
+ directMemoryUsageReporter.start();
Review Comment:
Could add test unit case to test the funcation of directMemoryUsageReporter?
Or I could try later.
@rickyma
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServer.java:
##########
@@ -142,6 +142,7 @@ public void start() throws Exception {
initMetricsReporter();
registerHeartBeat.startHeartBeat();
+ directMemoryUsageReporter.start();
Review Comment:
Could add test unit case to test the funcation of directMemoryUsageReporter?
Or I could try later.
##########
server/src/main/java/org/apache/uniffle/server/NettyDirectMemoryTracker.java:
##########
@@ -21,7 +21,7 @@
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
-import io.grpc.netty.shaded.io.netty.util.internal.PlatformDependent;
+import io.netty.util.internal.PlatformDependent;
Review Comment:
What is the difference between the two PlatformDependent?
Or is there problem using 'io.grpc.netty.shaded.io.netty.util.internal.PlatformDependent'
##########
server/src/main/java/org/apache/uniffle/server/NettyDirectMemoryTracker.java:
##########
@@ -21,7 +21,7 @@
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
-import io.grpc.netty.shaded.io.netty.util.internal.PlatformDependent;
+import io.netty.util.internal.PlatformDependent;
Review Comment:
What is the difference between the two PlatformDependent?
Or is there problem using 'io.grpc.netty.shaded.io.netty.util.internal.PlatformDependent'
@rickyma
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1189][FOLLOWUP] fix(server): Start NettyDirectMemoryTracker. [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1432:
URL: https://github.com/apache/incubator-uniffle/pull/1432#issuecomment-1888529195
> Could add test unit case to test the funcation of directMemoryUsageReporter? Or I could try later.
>
> @rickyma
Please add the unit tests, thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1189][FOLLOWUP] fix(server): Start NettyDirectMemoryTracker. [incubator-uniffle]
Posted by "lifeSo (via GitHub)" <gi...@apache.org>.
lifeSo commented on PR #1432:
URL: https://github.com/apache/incubator-uniffle/pull/1432#issuecomment-1888498230
@rickyma
What is the difference between the two PlatformDependent?
Or is there problem using 'io.grpc.netty.shaded.io.netty.util.internal.PlatformDependent'
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1189][FOLLOWUP] fix(server): Start NettyDirectMemoryTracker. [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1432:
URL: https://github.com/apache/incubator-uniffle/pull/1432#issuecomment-1886060483
@lifeSo Could you help review this pr?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1189][FOLLOWUP] fix(server): Start NettyDirectMemoryTracker. [incubator-uniffle]
Posted by "lifeSo (via GitHub)" <gi...@apache.org>.
lifeSo commented on PR #1432:
URL: https://github.com/apache/incubator-uniffle/pull/1432#issuecomment-1888482189
> > @lifeSo Could you help review this pr?
>
> ping @lifeSo
I reviewed yesterday, I try another way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1189][FOLLOWUP] fix(server): Start NettyDirectMemoryTracker. [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1432:
URL: https://github.com/apache/incubator-uniffle/pull/1432#issuecomment-1888527206
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1189][FOLLOWUP] fix(server): Start NettyDirectMemoryTracker. [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi merged PR #1432:
URL: https://github.com/apache/incubator-uniffle/pull/1432
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1189][FOLLOWUP] fix(server): Start NettyDirectMemoryTracker. [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1432:
URL: https://github.com/apache/incubator-uniffle/pull/1432#issuecomment-1884428469
PTAL @jerqi @zuston @lifeSo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1189][FOLLOWUP] fix(server): Start NettyDirectMemoryTracker. [incubator-uniffle]
Posted by "lifeSo (via GitHub)" <gi...@apache.org>.
lifeSo commented on PR #1432:
URL: https://github.com/apache/incubator-uniffle/pull/1432#issuecomment-1888604632
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1189][FOLLOWUP] fix(server): Start NettyDirectMemoryTracker. [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1432:
URL: https://github.com/apache/incubator-uniffle/pull/1432#issuecomment-1888483162
> > > @lifeSo Could you help review this pr?
> >
> >
> > ping @lifeSo
>
> I reviewed yesterday, I try another way.
>
> 
You should send the review information, otherwise others can't see it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1189][FOLLOWUP] fix(server): Start NettyDirectMemoryTracker. [incubator-uniffle]
Posted by "lifeSo (via GitHub)" <gi...@apache.org>.
lifeSo commented on PR #1432:
URL: https://github.com/apache/incubator-uniffle/pull/1432#issuecomment-1888498044
@rickyma
All is ok, but could add unit test case to test the tracker ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1189][FOLLOWUP] fix(server): Start NettyDirectMemoryTracker. [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #1432:
URL: https://github.com/apache/incubator-uniffle/pull/1432#issuecomment-1888336245
> @lifeSo Could you help review this pr?
ping @lifeSo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1189][FOLLOWUP] fix(server): Start NettyDirectMemoryTracker. [incubator-uniffle]
Posted by "lifeSo (via GitHub)" <gi...@apache.org>.
lifeSo commented on PR #1432:
URL: https://github.com/apache/incubator-uniffle/pull/1432#issuecomment-1888497260
> > @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1189][FOLLOWUP] fix(server): Start NettyDirectMemoryTracker. [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1432:
URL: https://github.com/apache/incubator-uniffle/pull/1432#issuecomment-1890398611
@lifeSo Can you approve this PR? You can add UTs later. This PR is only for starting NettyDirectMemoryTracker.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org