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 15:29:22 UTC

[PR] [MINOR] fix(server): Correct warning log for missing index when getLocalShuffleIndex [incubator-uniffle]

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

   <!--
   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?
   
   Correct the warning log.
   
   ### Why are the changes needed?
   
   The shuffle data could be exist in the buffer pool and have not flush to local disk yet, so the log is not correct.
   
   Fix: # (issue)
   
   ### Does this PR introduce _any_ user-facing change?
   
   No.
   
   ### How was this patch tested?
   
   No need.


-- 
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(server): Correct warning log for missing index when getLocalShuffleIndex [incubator-uniffle]

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

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

   > There are 3 flaky tests failed
   > 
   > * ShuffleServerMetricsTest, I resolved by [[MINOR] test: Fix flaky test about ShuffleServerMetricsTest#testStorageCounter #2142](https://github.com/apache/incubator-uniffle/pull/2142)
   > * AccessClusterTest cause test jvm crash, so many opening PR encountered this flaky test.
   > * QuorumTest, a famous flaky test.
   > 
   > ```
   > Error:  Tests run: 7, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 1.056 s <<< FAILURE! - in org.apache.uniffle.server.ShuffleServerMetricsTest
   > Error:  testStorageCounter  Time elapsed: 0.009 s  <<< FAILURE!
   > org.opentest4j.AssertionFailedError: expected: <1.0> but was: <3.0>
   > 	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
   > 	at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
   > 	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:86)
   > 	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:81)
   > 	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1010)
   > 	at org.apache.uniffle.server.ShuffleServerMetricsTest.testStorageCounter(ShuffleServerMetricsTest.java:176)
   > ...
   > 
   > org.apache.uniffle.test.AccessClusterTest
   > 
   > ...
   > Error:  Tests run: 14, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 228.081 s <<< FAILURE! - in org.apache.uniffle.test.QuorumTest
   > Error:  case10  Time elapsed: 184.154 s  <<< FAILURE!
   > org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
   > ```
   
   Could you help fix these flaky tests? @maobaolong 


-- 
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(server): Correct warning log for missing index when getLocalShuffleIndex [incubator-uniffle]

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

maobaolong commented on PR #2144:
URL: https://github.com/apache/incubator-uniffle/pull/2144#issuecomment-2375618631

   There are 3 flaky tests failed
   
   - ShuffleServerMetricsTest, I resolved by #2142
   - AccessClusterTest cause test jvm crash, so many opening PR encountered this flaky test.
   - QuorumTest, a famous flaky test.
   
   ```
   Error:  Tests run: 7, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 1.056 s <<< FAILURE! - in org.apache.uniffle.server.ShuffleServerMetricsTest
   Error:  testStorageCounter  Time elapsed: 0.009 s  <<< FAILURE!
   org.opentest4j.AssertionFailedError: expected: <1.0> but was: <3.0>
   	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
   	at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
   	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:86)
   	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:81)
   	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1010)
   	at org.apache.uniffle.server.ShuffleServerMetricsTest.testStorageCounter(ShuffleServerMetricsTest.java:176)
   ...
   
   org.apache.uniffle.test.AccessClusterTest
   
   ...
   Error:  Tests run: 14, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 228.081 s <<< FAILURE! - in org.apache.uniffle.test.QuorumTest
   Error:  case10  Time elapsed: 184.154 s  <<< FAILURE!
   org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
   ```


-- 
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(server): Correct warning log for missing index when getLocalShuffleIndex [incubator-uniffle]

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

maobaolong closed pull request #2144: [MINOR] fix(server): Correct warning log for missing index when getLocalShuffleIndex
URL: https://github.com/apache/incubator-uniffle/pull/2144


-- 
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(server): Correct warning log for missing index when getLocalShuffleIndex [incubator-uniffle]

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

maobaolong commented on PR #2144:
URL: https://github.com/apache/incubator-uniffle/pull/2144#issuecomment-2372661352

   @jerqi @zuston @advancedxy Would you like to take a look at this PR, this log make us afraid, after go through the code and our test context, I believe the data is in the buffer pool only, so I'd like to improve the log.


-- 
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(server): Correct warning log for missing index when getLocalShuffleIndex [incubator-uniffle]

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

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


##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java:
##########
@@ -1241,7 +1241,8 @@ public void getLocalShuffleIndex(
           reply = builder.build();
         } catch (FileNotFoundException indexFileNotFoundException) {
           LOG.warn(
-              "Index file for {} is not found, maybe the data has been flushed to cold storage.",
+              "Index file for {} is not found, maybe the data has been flushed to cold storage "
+                  + "or still in memory buffer pool.",

Review Comment:
   Got it, thanks for your explanation. BTW, the another read headler for partition spilt should be introduced to avoid too much unnecessary request. Anyway, current impl is OK for this case, let's optimize this in the later days.



-- 
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(server): Correct warning log for missing index when getLocalShuffleIndex [incubator-uniffle]

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

maobaolong closed pull request #2144: [MINOR] fix(server): Correct warning log for missing index when getLocalShuffleIndex
URL: https://github.com/apache/incubator-uniffle/pull/2144


-- 
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(server): Correct warning log for missing index when getLocalShuffleIndex [incubator-uniffle]

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

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


##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java:
##########
@@ -1241,7 +1241,8 @@ public void getLocalShuffleIndex(
           reply = builder.build();
         } catch (FileNotFoundException indexFileNotFoundException) {
           LOG.warn(
-              "Index file for {} is not found, maybe the data has been flushed to cold storage.",
+              "Index file for {} is not found, maybe the data has been flushed to cold storage "
+                  + "or still in memory buffer pool.",

Review Comment:
   `or still in memory buffer pool` -> or `partial data still in another shuffle server(when partition split is enabled)`
   
   How about 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: 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(server): Correct warning log for missing index when getLocalShuffleIndex [incubator-uniffle]

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

maobaolong commented on code in PR #2144:
URL: https://github.com/apache/incubator-uniffle/pull/2144#discussion_r1776441287


##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java:
##########
@@ -1241,7 +1241,8 @@ public void getLocalShuffleIndex(
           reply = builder.build();
         } catch (FileNotFoundException indexFileNotFoundException) {
           LOG.warn(
-              "Index file for {} is not found, maybe the data has been flushed to cold storage.",
+              "Index file for {} is not found, maybe the data has been flushed to cold storage "
+                  + "or still in memory buffer pool.",

Review Comment:
   @zuston It is what you said in the common case, but for the `partition split` case, the server 0 store the partial data of the partition in the memory, and server 1 store the other part of partition data in the memory too. then the client could read data by the `LocalFileClientReadHandler` from server 0, but server 0 has no index file, finally, the client read data from the memory of server 1.
   
   It works when `MultiReplicaClientReadHandler` contains multiply `ComposedClientReadHandler` for multiply 
    partition `replica` or `slice`, and each `ComposedClientReadHandler` contains two  read handlers for two `tier` which are `MemoryClientReadHandler` and `getLocalfileClientReaderHandler`. 
   
   This is interesting and I learn that spend much time 😄.



-- 
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(server): Correct warning log for missing index when getLocalShuffleIndex [incubator-uniffle]

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

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


##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java:
##########
@@ -1241,7 +1241,8 @@ public void getLocalShuffleIndex(
           reply = builder.build();
         } catch (FileNotFoundException indexFileNotFoundException) {
           LOG.warn(
-              "Index file for {} is not found, maybe the data has been flushed to cold storage.",
+              "Index file for {} is not found, maybe the data has been flushed to cold storage "
+                  + "or still in memory buffer pool.",

Review Comment:
   From my sight, the index file not found means the data loss or partial memory data flushed into the cold storage.



-- 
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(server): Correct warning log for missing index when getLocalShuffleIndex [incubator-uniffle]

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

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

   ## Test Results
    2 737 files   - 145   2 737 suites   - 145   4h 54m 43s :stopwatch: - 1h 4m 8s
    1 019 tests ±  0   1 015 :white_check_mark:  -   3   1 :zzz: ±0   3 :x: + 3 
   12 551 runs   - 317  12 525 :white_check_mark:  - 328  15 :zzz: ±0  11 :x: +11 
   
   For more details on these failures, see [this check](https://github.com/apache/incubator-uniffle/runs/30595473634).
   
   Results for commit 602c4cac. ± Comparison against base commit 3b016842.
   
   [test-results]:data:application/gzip;base64,H4sIADnh8mYC/03Myw6DIBCF4VcxrLvgOmJfxsAoCamXBmFl+u4dW0tdno/h31mI07ixeyNb1d4atpWYr3soyeW4LiSiBauI6D0fF4KL7jf7rSB+zfztEZ+HVQguTgQ10o8prYmEk6SyfKrSGHHOWpVGmmpntcJZFfXXtYrrPMdMgwGXqNEhD75DoQEsQgDrTae95yPYwWsdlGKvN5ND5pUVAQAA
   


-- 
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(server): Correct warning log for missing index when getLocalShuffleIndex [incubator-uniffle]

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

maobaolong commented on PR #2144:
URL: https://github.com/apache/incubator-uniffle/pull/2144#issuecomment-2381051003

   @zuston Thanks for your suggestion, I modified the output follow your 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(server): Correct warning log for missing index when getLocalShuffleIndex [incubator-uniffle]

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

maobaolong closed pull request #2144: [MINOR] fix(server): Correct warning log for missing index when getLocalShuffleIndex
URL: https://github.com/apache/incubator-uniffle/pull/2144


-- 
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(server): Correct warning log for missing index when getLocalShuffleIndex [incubator-uniffle]

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

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


##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java:
##########
@@ -1241,7 +1241,8 @@ public void getLocalShuffleIndex(
           reply = builder.build();
         } catch (FileNotFoundException indexFileNotFoundException) {
           LOG.warn(
-              "Index file for {} is not found, maybe the data has been flushed to cold storage.",
+              "Index file for {} is not found, maybe the data has been flushed to cold storage "
+                  + "or still in memory buffer pool.",

Review Comment:
   memory buffer pool means what? In linux page cache or what? 



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