You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "maobaolong (via GitHub)" <gi...@apache.org> on 2024/09/24 14:51:27 UTC

[PR] [MINOR] fix: fix log content of MultiReplicaClientReadHandler#readShuffleData [incubator-uniffle]

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

   <!--
   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?
   
   fix log content of MultiReplicaClientReadHandler#readShuffleData to different content for last server and others.
   
   ### Why are the changes needed?
   
   The existing log can make user stress, as we have multi replica or servers to handle shuffle data read, so we should warn to user until we cannot get all blocks from last server.
   
   ### Does this PR introduce _any_ user-facing change?
   
   Yes, the read task log will be more friendly.
   
   ### How was this patch tested?
   
   Test locally.


-- 
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] [MINOR] fix: fix log content of MultiReplicaClientReadHandler#readShuffleData [incubator-uniffle]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2143:
URL: https://github.com/apache/incubator-uniffle/pull/2143#issuecomment-2371619602

   ## Test Results
    2 815 files   -  67   2 815 suites   - 67   5h 31m 26s :stopwatch: - 27m 25s
    1 019 tests ±  0   1 015 :white_check_mark:  -   3   1 :zzz: ±0  3 :x: +3 
   12 716 runs   - 152  12 694 :white_check_mark:  - 159  15 :zzz: ±0  7 :x: +7 
   
   For more details on these failures, see [this check](https://github.com/apache/incubator-uniffle/runs/30593492723).
   
   Results for commit 8adc83ca. ± Comparison against base commit 3b016842.
   
   [test-results]:data:application/gzip;base64,H4sIAA7Z8mYC/03MTQ7CIBCG4asY1i46M6WAlzFkWhJifwyFlfHuTrXFrsj7DPleKsRxWNXtghb09aLWEvO5+5J8jsssAs7aTkjuefsBDbgj72th/pn+2yM+N6sQfBwFqMKQ0pJEGpFU5u8qGuj2rKvYubbavqoP2FfN0edRXqYpZgllfc+W2Ht0AwJrajmQZ4Tg0DsiI68hF9T7A/CjBcIUAQAA
   


-- 
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] [MINOR] fix: fix log content of MultiReplicaClientReadHandler#readShuffleData [incubator-uniffle]

Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #2143:
URL: https://github.com/apache/incubator-uniffle/pull/2143#discussion_r1776421682


##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/MultiReplicaClientReadHandler.java:
##########
@@ -76,10 +76,22 @@ public ShuffleDataResult readShuffleData() {
           RssUtils.checkProcessedBlockIds(blockIdBitmap, processedBlockIds);
           return result;
         } catch (RssException e) {
-          LOG.warn(
-              "Finished read from [{}], but haven't finished read all the blocks.",
-              shuffleServerInfos.get(readHandlerIndex).getId(),
-              e);
+          // If this is not the last server, then read next.
+          if (readHandlerIndex < handlers.size() - 1) {
+            LOG.info(
+                "Finished read from {}/{} [{}], haven't finished read all the blocks. Will read from next server {}",
+                readHandlerIndex + 1,
+                handlers.size(),
+                shuffleServerInfos.get(readHandlerIndex).getId(),
+                shuffleServerInfos.get(readHandlerIndex + 1).getId());

Review Comment:
   Maybe you should reserve the `e.getMessage` to help debug



-- 
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] [MINOR] fix: fix log content of MultiReplicaClientReadHandler#readShuffleData [incubator-uniffle]

Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on PR #2143:
URL: https://github.com/apache/incubator-uniffle/pull/2143#issuecomment-2376004920

   @zuston Thanks for your review and suggestion, PTAL.


-- 
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] [MINOR] fix: fix log content of MultiReplicaClientReadHandler#readShuffleData [incubator-uniffle]

Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston merged PR #2143:
URL: https://github.com/apache/incubator-uniffle/pull/2143


-- 
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] [MINOR] fix: fix log content of MultiReplicaClientReadHandler#readShuffleData [incubator-uniffle]

Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on code in PR #2143:
URL: https://github.com/apache/incubator-uniffle/pull/2143#discussion_r1776427014


##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/MultiReplicaClientReadHandler.java:
##########
@@ -76,10 +76,22 @@ public ShuffleDataResult readShuffleData() {
           RssUtils.checkProcessedBlockIds(blockIdBitmap, processedBlockIds);
           return result;
         } catch (RssException e) {
-          LOG.warn(
-              "Finished read from [{}], but haven't finished read all the blocks.",
-              shuffleServerInfos.get(readHandlerIndex).getId(),
-              e);
+          // If this is not the last server, then read next.
+          if (readHandlerIndex < handlers.size() - 1) {
+            LOG.info(
+                "Finished read from {}/{} [{}], haven't finished read all the blocks. Will read from next server {}",
+                readHandlerIndex + 1,
+                handlers.size(),
+                shuffleServerInfos.get(readHandlerIndex).getId(),
+                shuffleServerInfos.get(readHandlerIndex + 1).getId());

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