You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@uniffle.apache.org by "somelovelanguage (via GitHub)" <gi...@apache.org> on 2024/02/06 06:35:36 UTC

[PR] [FEATURE] Provide dedicated unregister app rpc interface [incubator-uniffle]

somelovelanguage opened a new pull request, #1510:
URL: https://github.com/apache/incubator-uniffle/pull/1510

   <!--
   1. Title: [#<issue>] <type>(<scope>): <subject>
      Examples:
        - "[#123] feat(operator): support xxx"
        - "[#233] fix: check null before access result in xxx"
        - "[MINOR] refactor: fix typo in variable name"
        - "[MINOR] docs: fix typo in README"
        - "[#255] test: fix flaky test NameOfTheTest"
      Reference: https://www.conventionalcommits.org/en/v1.0.0/
   2. Contributor guidelines:
      https://github.com/apache/incubator-uniffle/blob/master/CONTRIBUTING.md
   3. If the PR is unfinished, please mark this PR as draft.
   -->
   
   ### What changes were proposed in this pull request?
   
   Introduce dedicated unregisterApp rpc interface which is only called once when unregister shuffle
   
   
   ### Why are the changes needed?
   
   
   Fix: [#1476](https://github.com/apache/incubator-uniffle/issues/1476)
   
   ### Does this PR introduce _any_ user-facing change?
   
   
   No.
   
   ### How was this patch tested?
   
   UT
   


-- 
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] [#1476] feat(spark): Provide dedicated unregister app rpc interface [incubator-uniffle]

Posted by "somelovelanguage (via GitHub)" <gi...@apache.org>.

somelovelanguage commented on code in PR #1510:
URL: https://github.com/apache/incubator-uniffle/pull/1510#discussion_r1480856352


##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java:
##########
@@ -99,6 +102,37 @@ public ShuffleServerGrpcService(ShuffleServer shuffleServer) {
     this.shuffleServer = shuffleServer;
   }
 
+  @Override
+  public void unregisterApp(
+      RssProtos.AppUnregisterRequest request,
+      StreamObserver<RssProtos.AppUnregisterResponse> responseStreamObserver) {
+    String appId = request.getAppId();
+
+    StatusCode result = StatusCode.SUCCESS;
+    String responseMessage = "OK";
+    try {
+      Map<Integer, RangeMap<Integer, ShuffleBuffer>> shuffleIdToBuffers =
+          shuffleServer.getShuffleBufferManager().getBufferPool().get(appId);
+      if (shuffleIdToBuffers != null) {
+        HashSet<Integer> shuffleIds = Sets.newHashSet(shuffleIdToBuffers.keySet());
+        shuffleIds.forEach(
+            shuffleId ->
+                shuffleServer.getShuffleTaskManager().removeShuffleDataAsync(appId, shuffleId));

Review Comment:
   Maybe like this.
   ```java
   public void removeShuffleDataAsync(String appId) {
       Map<Integer, RangeMap<Integer, ShuffleBuffer>> shuffleIdToBuffers =
               this.shuffleBufferManager.getBufferPool().get(appId);
       if (shuffleIdToBuffers != null) {
         HashSet<Integer> shuffleIds = Sets.newHashSet(shuffleIdToBuffers.keySet());
         expiredAppIdQueue.add(new ShufflePurgeEvent(appId, getUserByAppId(appId) , (List<Integer>) shuffleIds);
       }
     }
   ```



-- 
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] [#1476] feat(spark): Provide dedicated unregister app rpc interface [incubator-uniffle]

Posted by "somelovelanguage (via GitHub)" <gi...@apache.org>.

somelovelanguage commented on code in PR #1510:
URL: https://github.com/apache/incubator-uniffle/pull/1510#discussion_r1479564906


##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java:
##########
@@ -99,6 +102,37 @@ public ShuffleServerGrpcService(ShuffleServer shuffleServer) {
     this.shuffleServer = shuffleServer;
   }
 
+  @Override
+  public void unregisterApp(
+      RssProtos.AppUnregisterRequest request,
+      StreamObserver<RssProtos.AppUnregisterResponse> responseStreamObserver) {
+    String appId = request.getAppId();
+
+    StatusCode result = StatusCode.SUCCESS;
+    String responseMessage = "OK";
+    try {
+      Map<Integer, RangeMap<Integer, ShuffleBuffer>> shuffleIdToBuffers =
+          shuffleServer.getShuffleBufferManager().getBufferPool().get(appId);
+      if (shuffleIdToBuffers != null) {
+        HashSet<Integer> shuffleIds = Sets.newHashSet(shuffleIdToBuffers.keySet());
+        shuffleIds.forEach(
+            shuffleId ->
+                shuffleServer.getShuffleTaskManager().removeShuffleDataAsync(appId, shuffleId));

Review Comment:
   I tried to do this at first, but I found that when processing AppPurgeEvent, it will first determine whether the App has expired. 
   ```java
    // the thread for clear expired resources
   
   // 1
       Runnable clearResourceRunnable =
           () -> {
             while (true) {
               PurgeEvent event = null;
               try {
                 event = expiredAppIdQueue.take();
                 long startTime = System.currentTimeMillis();
                 if (event instanceof AppPurgeEvent) {
                   removeResources(event.getAppId(), true);
                   double usedTime =
                       (System.currentTimeMillis() - startTime) / Constants.MILLION_SECONDS_PER_SECOND;
                   ShuffleServerMetrics.summaryTotalRemoveResourceTime.observe(usedTime);
                 }
   // 2
   public void removeResources(String appId, boolean checkAppExpired) {
       Lock lock = getAppLock(appId);
       try {
         lock.lock();
         LOG.info("Start remove resource for appId[" + appId + "]");
         if (checkAppExpired && !isAppExpired(appId)) {
           LOG.info(
               "It seems that this appId[{}] has registered a new shuffle, just ignore this AppPurgeEvent event.",
               appId);
           return;
         }
   
   // 3
    private boolean isAppExpired(String appId) {
       if (shuffleTaskInfos.get(appId) == null) {
         return true;
       }
       return System.currentTimeMillis() - shuffleTaskInfos.get(appId).getCurrentTimes()
           > appExpiredWithoutHB;
     }
   ```
   
   According to my understanding, in this case, the App is not necessarily expired, so it may not be able to play the role of unregister shuffle. If my understanding is wrong, please point me out. 
   
   ```java
   public void removeShuffleDataAsync(String appId) {
       shuffleTaskInfos.remove(appId);
       expiredAppIdQueue.add(
               new AppPurgeEvent(appId, getUserByAppId(appId));
     }
   
   ```
   Maybe we can do this(remove the appId first and then do what you said) , WDYT?
   



-- 
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] [#1476] feat(spark): Provide dedicated unregister app rpc interface [incubator-uniffle]

Posted by "somelovelanguage (via GitHub)" <gi...@apache.org>.

somelovelanguage commented on code in PR #1510:
URL: https://github.com/apache/incubator-uniffle/pull/1510#discussion_r1481079187


##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java:
##########
@@ -99,6 +102,37 @@ public ShuffleServerGrpcService(ShuffleServer shuffleServer) {
     this.shuffleServer = shuffleServer;
   }
 
+  @Override
+  public void unregisterApp(
+      RssProtos.AppUnregisterRequest request,
+      StreamObserver<RssProtos.AppUnregisterResponse> responseStreamObserver) {
+    String appId = request.getAppId();
+
+    StatusCode result = StatusCode.SUCCESS;
+    String responseMessage = "OK";
+    try {
+      Map<Integer, RangeMap<Integer, ShuffleBuffer>> shuffleIdToBuffers =
+          shuffleServer.getShuffleBufferManager().getBufferPool().get(appId);
+      if (shuffleIdToBuffers != null) {
+        HashSet<Integer> shuffleIds = Sets.newHashSet(shuffleIdToBuffers.keySet());
+        shuffleIds.forEach(
+            shuffleId ->
+                shuffleServer.getShuffleTaskManager().removeShuffleDataAsync(appId, shuffleId));

Review Comment:
   I understand, I have completed the modification, please check it out, thanks!



-- 
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] [#1476] feat(spark): Provide dedicated unregister app rpc interface [incubator-uniffle]

Posted by "zuston (via GitHub)" <gi...@apache.org>.

zuston commented on PR #1510:
URL: https://github.com/apache/incubator-uniffle/pull/1510#issuecomment-1933221634

   Merged. Thanks for your contribution @somelovelanguage .
   
   Welcome to Uniffle community! 🎉


-- 
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] [FEATURE] Provide dedicated unregister app rpc interface [incubator-uniffle]

Posted by "zuston (via GitHub)" <gi...@apache.org>.

zuston commented on code in PR #1510:
URL: https://github.com/apache/incubator-uniffle/pull/1510#discussion_r1479360091


##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java:
##########
@@ -99,6 +102,37 @@ public ShuffleServerGrpcService(ShuffleServer shuffleServer) {
     this.shuffleServer = shuffleServer;
   }
 
+  @Override
+  public void unregisterApp(
+      RssProtos.AppUnregisterRequest request,
+      StreamObserver<RssProtos.AppUnregisterResponse> responseStreamObserver) {
+    String appId = request.getAppId();
+
+    StatusCode result = StatusCode.SUCCESS;
+    String responseMessage = "OK";
+    try {
+      Map<Integer, RangeMap<Integer, ShuffleBuffer>> shuffleIdToBuffers =
+          shuffleServer.getShuffleBufferManager().getBufferPool().get(appId);
+      if (shuffleIdToBuffers != null) {
+        HashSet<Integer> shuffleIds = Sets.newHashSet(shuffleIdToBuffers.keySet());
+        shuffleIds.forEach(
+            shuffleId ->
+                shuffleServer.getShuffleTaskManager().removeShuffleDataAsync(appId, shuffleId));

Review Comment:
   I think the deleting all shuffleIds  one by one is not a good way.
   
   You could follow the next step to extend
   1. create a new method in ShuffleTaskManager, like this:
   ```
   public void removeShuffleDataAsync(String appId) {
       expiredAppIdQueue.add(
           new AppPurgeEvent(appId, getUserByAppId(appId));
     }
   ```
   2. use above method in grpc service.



-- 
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] [#1476] feat(spark): Provide dedicated unregister app rpc interface [incubator-uniffle]

Posted by "somelovelanguage (via GitHub)" <gi...@apache.org>.

somelovelanguage commented on code in PR #1510:
URL: https://github.com/apache/incubator-uniffle/pull/1510#discussion_r1479565507


##########
proto/src/main/proto/Rss.proto:
##########
@@ -26,6 +26,7 @@ package rss.common;
 service ShuffleServer {
   rpc registerShuffle (ShuffleRegisterRequest) returns (ShuffleRegisterResponse);
   rpc unregisterShuffle(ShuffleUnregisterRequest) returns (ShuffleUnregisterResponse);
+  rpc unregisterApp(AppUnregisterRequest) returns (AppUnregisterResponse);

Review Comment:
   done.



-- 
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] [#1476] feat(spark): Provide dedicated unregister app rpc interface [incubator-uniffle]

Posted by "zuston (via GitHub)" <gi...@apache.org>.

zuston merged PR #1510:
URL: https://github.com/apache/incubator-uniffle/pull/1510


-- 
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] [FEATURE] Provide dedicated unregister app rpc interface [incubator-uniffle]

Posted by "jerqi (via GitHub)" <gi...@apache.org>.

jerqi commented on code in PR #1510:
URL: https://github.com/apache/incubator-uniffle/pull/1510#discussion_r1479358984


##########
proto/src/main/proto/Rss.proto:
##########
@@ -26,6 +26,7 @@ package rss.common;
 service ShuffleServer {
   rpc registerShuffle (ShuffleRegisterRequest) returns (ShuffleRegisterResponse);
   rpc unregisterShuffle(ShuffleUnregisterRequest) returns (ShuffleUnregisterResponse);
+  rpc unregisterApp(AppUnregisterRequest) returns (AppUnregisterResponse);

Review Comment:
   Prefer using `unregisterShuffleByAppId(String appId);`
   Because we don't have registerApp interface. It's weird to use interface `unregisterApp`.
   



-- 
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] [#1476] feat(spark): Provide dedicated unregister app rpc interface [incubator-uniffle]

Posted by "zuston (via GitHub)" <gi...@apache.org>.

zuston commented on code in PR #1510:
URL: https://github.com/apache/incubator-uniffle/pull/1510#discussion_r1480788538


##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java:
##########
@@ -99,6 +102,37 @@ public ShuffleServerGrpcService(ShuffleServer shuffleServer) {
     this.shuffleServer = shuffleServer;
   }
 
+  @Override
+  public void unregisterApp(
+      RssProtos.AppUnregisterRequest request,
+      StreamObserver<RssProtos.AppUnregisterResponse> responseStreamObserver) {
+    String appId = request.getAppId();
+
+    StatusCode result = StatusCode.SUCCESS;
+    String responseMessage = "OK";
+    try {
+      Map<Integer, RangeMap<Integer, ShuffleBuffer>> shuffleIdToBuffers =
+          shuffleServer.getShuffleBufferManager().getBufferPool().get(appId);
+      if (shuffleIdToBuffers != null) {
+        HashSet<Integer> shuffleIds = Sets.newHashSet(shuffleIdToBuffers.keySet());
+        shuffleIds.forEach(
+            shuffleId ->
+                shuffleServer.getShuffleTaskManager().removeShuffleDataAsync(appId, shuffleId));

Review Comment:
   We should preserve these logic that solved the bug of #1212 . 
   
   How about introducing new event named AppUnregisterPurgeEvent? 



-- 
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] [#1476] feat(spark): Provide dedicated unregister app rpc interface [incubator-uniffle]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.

github-actions[bot] commented on PR #1510:
URL: https://github.com/apache/incubator-uniffle/pull/1510#issuecomment-1929016141

   ## Test Results
   2 287 files  2 287 suites   4h 28m 24s :stopwatch:
     819 tests   818 :white_check_mark:  1 :zzz: 0 :x:
   9 086 runs  9 073 :white_check_mark: 13 :zzz: 0 :x:
   
   Results for commit a3a76f83.
   
   [test-results]:data:application/gzip;base64,H4sIACHvwWUC/03MQQ6DIBCF4asY1l0AY2Cml2mASkKq0iCsmt69aJC6fB/D/2E+zNPG7oOUqG8D20rI1/0syeQQ1ypCCT5Wqu95v0BB53psxbmD8E+v8N5/dfAmzBV4hymlmJqksu5N4qjaOpvENXRqzQ6X5rGvSReXJeQ6mAGjlUeQZJUQCE6PlrwmA4ASvLUSHHAy7PsDo8qGiBABAAA=
   


-- 
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] [#1476] feat(spark): Provide dedicated unregister app rpc interface [incubator-uniffle]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.

codecov-commenter commented on PR #1510:
URL: https://github.com/apache/incubator-uniffle/pull/1510#issuecomment-1929283421

   ## [Codecov](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1510?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   Attention: `44 lines` in your changes are missing coverage. Please review.
   > Comparison is base [(`e3b4618`)](https://app.codecov.io/gh/apache/incubator-uniffle/commit/e3b4618df8f06af1610b40788164617af56b9e93?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 0.00% compared to head [(`a89fd66`)](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1510?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 55.19%.
   > Report is 8 commits behind head on master.
   
   | [Files](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [...pache/uniffle/server/ShuffleServerGrpcService.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlU2VydmVyR3JwY1NlcnZpY2UuamF2YQ==) | 0.00% | [19 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [...ffle/client/impl/grpc/ShuffleServerGrpcClient.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aW50ZXJuYWwtY2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NsaWVudC9pbXBsL2dycGMvU2h1ZmZsZVNlcnZlckdycGNDbGllbnQuamF2YQ==) | 0.00% | [14 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [...he/uniffle/client/impl/ShuffleWriteClientImpl.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NsaWVudC9pbXBsL1NodWZmbGVXcml0ZUNsaWVudEltcGwuamF2YQ==) | 75.00% | [4 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [...nt/request/RssUnregisterShuffleByAppIdRequest.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aW50ZXJuYWwtY2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NsaWVudC9yZXF1ZXN0L1Jzc1VucmVnaXN0ZXJTaHVmZmxlQnlBcHBJZFJlcXVlc3QuamF2YQ==) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [.../response/RssUnregisterShuffleByAppIdResponse.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aW50ZXJuYWwtY2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NsaWVudC9yZXNwb25zZS9Sc3NVbnJlZ2lzdGVyU2h1ZmZsZUJ5QXBwSWRSZXNwb25zZS5qYXZh) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   
   <details><summary>Additional details and impacted files</summary>
   
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #1510       +/-   ##
   =============================================
   + Coverage          0   55.19%   +55.19%     
   - Complexity        0     2808     +2808     
   =============================================
     Files             0      409      +409     
     Lines             0    22046    +22046     
     Branches          0     2082     +2082     
   =============================================
   + Hits              0    12169    +12169     
   - Misses            0     9119     +9119     
   - Partials          0      758      +758     
   ```
   
   
   
   </details>
   
   [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1510?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   


-- 
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] [#1476] feat(spark): Provide dedicated unregister app rpc interface [incubator-uniffle]

Posted by "somelovelanguage (via GitHub)" <gi...@apache.org>.

somelovelanguage commented on code in PR #1510:
URL: https://github.com/apache/incubator-uniffle/pull/1510#discussion_r1480852981


##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java:
##########
@@ -99,6 +102,37 @@ public ShuffleServerGrpcService(ShuffleServer shuffleServer) {
     this.shuffleServer = shuffleServer;
   }
 
+  @Override
+  public void unregisterApp(
+      RssProtos.AppUnregisterRequest request,
+      StreamObserver<RssProtos.AppUnregisterResponse> responseStreamObserver) {
+    String appId = request.getAppId();
+
+    StatusCode result = StatusCode.SUCCESS;
+    String responseMessage = "OK";
+    try {
+      Map<Integer, RangeMap<Integer, ShuffleBuffer>> shuffleIdToBuffers =
+          shuffleServer.getShuffleBufferManager().getBufferPool().get(appId);
+      if (shuffleIdToBuffers != null) {
+        HashSet<Integer> shuffleIds = Sets.newHashSet(shuffleIdToBuffers.keySet());
+        shuffleIds.forEach(
+            shuffleId ->
+                shuffleServer.getShuffleTaskManager().removeShuffleDataAsync(appId, shuffleId));

Review Comment:
   
   #1212 This is mainly due to the asynchronous event that once thought the App was no longer used, because re-register will be used again (resend heartbeat), so when removeResource will check again whether the App has expired.
   
   Considering this issue, here, we want to unregister all shuffles of the app at the **"current"** moment, which means that if` AppUnregisterPurgeEvent` is introduced, we need to pass all shuffleIds of the current moment into the event. If so, why don't we just use `ShufflePurgeEvent` and pass the shuffleIds into 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] [#1476] feat(spark): Provide dedicated unregister app rpc interface [incubator-uniffle]

Posted by "zuston (via GitHub)" <gi...@apache.org>.

zuston commented on code in PR #1510:
URL: https://github.com/apache/incubator-uniffle/pull/1510#discussion_r1481005843


##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java:
##########
@@ -99,6 +102,37 @@ public ShuffleServerGrpcService(ShuffleServer shuffleServer) {
     this.shuffleServer = shuffleServer;
   }
 
+  @Override
+  public void unregisterApp(
+      RssProtos.AppUnregisterRequest request,
+      StreamObserver<RssProtos.AppUnregisterResponse> responseStreamObserver) {
+    String appId = request.getAppId();
+
+    StatusCode result = StatusCode.SUCCESS;
+    String responseMessage = "OK";
+    try {
+      Map<Integer, RangeMap<Integer, ShuffleBuffer>> shuffleIdToBuffers =
+          shuffleServer.getShuffleBufferManager().getBufferPool().get(appId);
+      if (shuffleIdToBuffers != null) {
+        HashSet<Integer> shuffleIds = Sets.newHashSet(shuffleIdToBuffers.keySet());
+        shuffleIds.forEach(
+            shuffleId ->
+                shuffleServer.getShuffleTaskManager().removeShuffleDataAsync(appId, shuffleId));

Review Comment:
   > Considering this issue, here, we want to unregister all shuffles of the app at the "current" moment, which means that if AppUnregisterPurgeEvent is introduced, we need to pass all shuffleIds of the current moment into the event. If so, why don't we just use ShufflePurgeEvent and pass the shuffleIds into it.
   
   I means we could remove app without rechecking the heartbeat timeout.
   
   ![image](https://github.com/apache/incubator-uniffle/assets/8609142/6e787e5e-a4b6-41dd-9b1b-e300d47e472f)
   
   



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