You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celeborn.apache.org by GitBox <gi...@apache.org> on 2022/11/30 07:47:04 UTC

[GitHub] [incubator-celeborn] boneanxs opened a new pull request, #1028: [CELEBORN-63] Increase push data return reason types such as CONGESTION ect

boneanxs opened a new pull request, #1028:
URL: https://github.com/apache/incubator-celeborn/pull/1028

   # [FEATURE] Increase push data return reason types such as CONGESTION ect
   
   ### What changes were proposed in this pull request?
   Add following statuses to allow `ShuffleServer` return statuses to the client side.
   
   SUCCESS_FREE
   SUCCESS_MASTER_CONGESTED
   SUCCESS_SLAVE_CONGESTED
   
   ### Why are the changes needed?
   
   To support Rate Limit mechanism
   
   ### What are the items that need reviewer attention?
   No
   
   ### Related issues.
   
   
   ### Related pull requests.
   
   
   ### How was this patch tested?
   
   
   /cc @related-reviewer
   
   /assign @main-reviewer
   


-- 
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: dev-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] AngersZhuuuu commented on a diff in pull request #1028: [CELEBORN-63] Increase push data return reason types such as CONGESTION ect

Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on code in PR #1028:
URL: https://github.com/apache/incubator-celeborn/pull/1028#discussion_r1035641599


##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/PushDataHandler.scala:
##########
@@ -262,7 +262,7 @@ class PushDataHandler extends BaseMessageHandler with Logging {
         }
       })
     } else {
-      wrappedCallback.onSuccess(ByteBuffer.wrap(Array[Byte]()))
+      wrappedCallback.onSuccess(ByteBuffer.wrap(Array[Byte](StatusCode.SUCCESS_FREE.getValue)))

Review Comment:
   Should not update here



-- 
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: dev-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] AngersZhuuuu merged pull request #1028: [CELEBORN-63] Add CONGESTION related status codes

Posted by GitBox <gi...@apache.org>.
AngersZhuuuu merged PR #1028:
URL: https://github.com/apache/incubator-celeborn/pull/1028


-- 
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: dev-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] AngersZhuuuu commented on pull request #1028: [CELEBORN-63] Increase push data return reason types such as CONGESTION ect

Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on PR #1028:
URL: https://github.com/apache/incubator-celeborn/pull/1028#issuecomment-1331920871

   ping @waitinfuture 


-- 
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: dev-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] AngersZhuuuu commented on a diff in pull request #1028: [CELEBORN-63] Increase push data return reason types such as CONGESTION ect

Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on code in PR #1028:
URL: https://github.com/apache/incubator-celeborn/pull/1028#discussion_r1035648982


##########
common/src/main/scala/org/apache/celeborn/common/util/Utils.scala:
##########
@@ -890,6 +890,12 @@ object Utils extends Logging {
         StatusCode.WORKER_IN_BLACKLIST
       case 28 =>
         StatusCode.UNKNOWN_WORKER
+      case 30 =>
+        StatusCode.SUCCESS_FREE

Review Comment:
   PUSH_SUCCESS



##########
common/src/main/scala/org/apache/celeborn/common/util/Utils.scala:
##########
@@ -890,6 +890,12 @@ object Utils extends Logging {
         StatusCode.WORKER_IN_BLACKLIST
       case 28 =>
         StatusCode.UNKNOWN_WORKER
+      case 30 =>
+        StatusCode.SUCCESS_FREE
+      case 31 =>
+        StatusCode.SUCCESS_MASTER_CONGESTED

Review Comment:
   ditto



##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/PushDataHandler.scala:
##########
@@ -262,7 +262,7 @@ class PushDataHandler extends BaseMessageHandler with Logging {
         }
       })
     } else {
-      wrappedCallback.onSuccess(ByteBuffer.wrap(Array[Byte]()))
+      wrappedCallback.onSuccess(ByteBuffer.wrap(Array[Byte](StatusCode.SUCCESS_FREE.getValue)))

Review Comment:
   We should also change this in `pushMergedData`



##########
client/src/main/java/org/apache/celeborn/client/ShuffleClientImpl.java:
##########
@@ -591,6 +591,7 @@ public int pushOrMergeData(
             @Override
             public void onSuccess(ByteBuffer response) {
               pushState.inFlightBatches.remove(nextBatchId);
+              // TODO Need to handle CONGESTED & FREE statuses

Review Comment:
   Add comment about why not handle PUSH_SUCCES, also add in pushMergedData



##########
common/src/main/scala/org/apache/celeborn/common/util/Utils.scala:
##########
@@ -890,6 +890,12 @@ object Utils extends Logging {
         StatusCode.WORKER_IN_BLACKLIST
       case 28 =>
         StatusCode.UNKNOWN_WORKER
+      case 30 =>
+        StatusCode.SUCCESS_FREE
+      case 31 =>
+        StatusCode.SUCCESS_MASTER_CONGESTED
+      case 32 =>
+        StatusCode.SUCCESS_SLAVE_CONGESTED

Review Comment:
   ditto



-- 
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: dev-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] AngersZhuuuu commented on a diff in pull request #1028: [CELEBORN-63] Increase push data return reason types such as CONGESTION ect

Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on code in PR #1028:
URL: https://github.com/apache/incubator-celeborn/pull/1028#discussion_r1035643975


##########
common/src/main/java/org/apache/celeborn/common/protocol/message/StatusCode.java:
##########
@@ -55,7 +55,12 @@ public enum StatusCode {
   WORKER_IN_BLACKLIST(27),
   UNKNOWN_WORKER(28),
 
-  COMMIT_FILE_EXCEPTION(29);
+  COMMIT_FILE_EXCEPTION(29),
+
+  // Rate limit statuses
+  SUCCESS_FREE(30),

Review Comment:
   Can remove this



-- 
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: dev-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] AngersZhuuuu commented on a diff in pull request #1028: [CELEBORN-63] Increase push data return reason types such as CONGESTION ect

Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on code in PR #1028:
URL: https://github.com/apache/incubator-celeborn/pull/1028#discussion_r1035641599


##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/PushDataHandler.scala:
##########
@@ -262,7 +262,7 @@ class PushDataHandler extends BaseMessageHandler with Logging {
         }
       })
     } else {
-      wrappedCallback.onSuccess(ByteBuffer.wrap(Array[Byte]()))
+      wrappedCallback.onSuccess(ByteBuffer.wrap(Array[Byte](StatusCode.SUCCESS_FREE.getValue)))

Review Comment:
   Should not update here



-- 
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: dev-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] waitinfuture commented on pull request #1028: [CELEBORN-63] Add CONGESTION related status codes

Posted by GitBox <gi...@apache.org>.
waitinfuture commented on PR #1028:
URL: https://github.com/apache/incubator-celeborn/pull/1028#issuecomment-1332043347

   Plz fix style issues by ./dev/reformat


-- 
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: dev-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] boneanxs commented on pull request #1028: [CELEBORN-63] Add CONGESTION related status codes

Posted by GitBox <gi...@apache.org>.
boneanxs commented on PR #1028:
URL: https://github.com/apache/incubator-celeborn/pull/1028#issuecomment-1333033844

   done @waitinfuture @AngersZhuuuu 


-- 
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: dev-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org