You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by GitBox <gi...@apache.org> on 2023/01/12 12:33:07 UTC

[GitHub] [incubator-uniffle] kaijchen opened a new pull request, #478: [Test] Make LocalFileHandlerTestBase stateless

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

   ### What changes were proposed in this pull request?
   
   Make LocalFileHandlerTestBase stateless
   
   ### Why are the changes needed?
   
   Followup #473 
   
   ### Does this PR introduce _any_ user-facing change?
   
   No.
   
   ### How was this patch tested?
   
   CI.
   


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

[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #478: [Test] Assume unknown blockID in LocalFileHandlerTestBase

Posted by GitBox <gi...@apache.org>.
kaijchen commented on code in PR #478:
URL: https://github.com/apache/incubator-uniffle/pull/478#discussion_r1068959171


##########
storage/src/test/java/org/apache/uniffle/storage/handler/impl/LocalFileServerReadHandlerTest.java:
##########
@@ -80,7 +78,7 @@ public void testDataInconsistent() throws Exception {
     int readBufferSize = 13;
     int bytesPerSegment = ((readBufferSize / blockSize) + 1) * blockSize;
     List<byte[]> segments = LocalFileHandlerTestBase.calcSegmentBytes(expectedData,
-        bytesPerSegment, actualWriteDataBlock);
+        bytesPerSegment, blockIds.subList(0, actualWriteDataBlock));

Review Comment:
   @zuston because we need to get subList 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: 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

[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #478: [Test] Assume unknown blockID in LocalFileHandlerTestBase

Posted by GitBox <gi...@apache.org>.
kaijchen commented on code in PR #478:
URL: https://github.com/apache/incubator-uniffle/pull/478#discussion_r1070772391


##########
storage/src/test/java/org/apache/uniffle/storage/handler/impl/LocalFileHandlerTestBase.java:
##########
@@ -44,16 +44,13 @@
 public class LocalFileHandlerTestBase {
   private static AtomicLong ATOMIC_LONG = new AtomicLong(0L);
 
-  public static void reset() {
-    ATOMIC_LONG = new AtomicLong(0L);
-  }
-
-  public static void writeTestData(
+  public static List<Long> writeTestData(
       ShuffleWriteHandler writeHandler,
       int num, int length,
       Map<Long, byte[]> expectedData,
       Set<Long> expectedBlockIds) throws Exception {
     List<ShufflePartitionedBlock> blocks = Lists.newArrayList();
+    List<Long> blockIds = Lists.newArrayList();

Review Comment:
   Thanks, I got it when you mean `TreeSet`. However, I'm wondering if we should keep the current `writeTestData()` signature.



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

[GitHub] [incubator-uniffle] advancedxy commented on pull request #478: [Test] Assume unknown blockID in LocalFileHandlerTestBase

Posted by GitBox <gi...@apache.org>.
advancedxy commented on PR #478:
URL: https://github.com/apache/incubator-uniffle/pull/478#issuecomment-1384007315

   Merging this. Thanks @kaijchen @zuston 


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

[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #478: [Test] Assume unknown blockID in LocalFileHandlerTestBase

Posted by GitBox <gi...@apache.org>.
kaijchen commented on code in PR #478:
URL: https://github.com/apache/incubator-uniffle/pull/478#discussion_r1069576310


##########
storage/src/test/java/org/apache/uniffle/storage/handler/impl/LocalFileHandlerTestBase.java:
##########
@@ -44,16 +44,13 @@
 public class LocalFileHandlerTestBase {
   private static AtomicLong ATOMIC_LONG = new AtomicLong(0L);
 
-  public static void reset() {
-    ATOMIC_LONG = new AtomicLong(0L);
-  }
-
-  public static void writeTestData(
+  public static List<Long> writeTestData(
       ShuffleWriteHandler writeHandler,
       int num, int length,
       Map<Long, byte[]> expectedData,
       Set<Long> expectedBlockIds) throws Exception {
     List<ShufflePartitionedBlock> blocks = Lists.newArrayList();
+    List<Long> blockIds = Lists.newArrayList();

Review Comment:
   Can you give an example how to use `expectedBlockIds` in `calcSegmentBytes`?
   I think we should preserve order and exclude the last block.



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

[GitHub] [incubator-uniffle] zuston commented on pull request #478: [Test] Make LocalFileHandlerTestBase stateless

Posted by GitBox <gi...@apache.org>.
zuston commented on PR #478:
URL: https://github.com/apache/incubator-uniffle/pull/478#issuecomment-1380366585

   > just retrieve blocks from blockIdToData
   
   I prefer to this solution


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

[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #478: [Test] Assume unknown blockID in LocalFileHandlerTestBase

Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #478:
URL: https://github.com/apache/incubator-uniffle/pull/478#discussion_r1069563352


##########
storage/src/test/java/org/apache/uniffle/storage/handler/impl/LocalFileHandlerTestBase.java:
##########
@@ -44,16 +44,13 @@
 public class LocalFileHandlerTestBase {
   private static AtomicLong ATOMIC_LONG = new AtomicLong(0L);
 
-  public static void reset() {
-    ATOMIC_LONG = new AtomicLong(0L);
-  }
-
-  public static void writeTestData(
+  public static List<Long> writeTestData(
       ShuffleWriteHandler writeHandler,
       int num, int length,
       Map<Long, byte[]> expectedData,
       Set<Long> expectedBlockIds) throws Exception {
     List<ShufflePartitionedBlock> blocks = Lists.newArrayList();
+    List<Long> blockIds = Lists.newArrayList();

Review Comment:
   +1



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

[GitHub] [incubator-uniffle] kaijchen commented on pull request #478: [Test] Assume unknown blockID in LocalFileHandlerTestBase

Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #478:
URL: https://github.com/apache/incubator-uniffle/pull/478#issuecomment-1383572266

   Thanks @zuston and @advancedxy for the review.
   I have splitted `Split LocalFileHandlerTestBase#writeTestData()` into `generateBlocks()` and `writeTestData()`.
   How about that?


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

[GitHub] [incubator-uniffle] advancedxy commented on pull request #478: [Test] Make LocalFileHandlerTestBase stateless

Posted by GitBox <gi...@apache.org>.
advancedxy commented on PR #478:
URL: https://github.com/apache/incubator-uniffle/pull/478#issuecomment-1380331912

   Did a quick overview of the related code, I believe the better fix should be refactor `org.apache.uniffle.storage.handler.impl.LocalFileHandlerTestBase#calcSegmentBytes` by passing the correct `wroteBlockList` or just retrieve blocks from `blockIdToData`
   
   In a real environment, the blockId could be random and generated concurrently. `org.apache.uniffle.storage.handler.impl.LocalFileHandlerTestBase#ATOMIC_LONG` seems to be a good way to simulate it.
   
   also cc @zuston 


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

[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #478: [Test] Assume unknown blockID in LocalFileHandlerTestBase

Posted by GitBox <gi...@apache.org>.
advancedxy commented on code in PR #478:
URL: https://github.com/apache/incubator-uniffle/pull/478#discussion_r1070803904


##########
storage/src/test/java/org/apache/uniffle/storage/handler/impl/LocalFileHandlerTestBase.java:
##########
@@ -44,16 +44,13 @@
 public class LocalFileHandlerTestBase {
   private static AtomicLong ATOMIC_LONG = new AtomicLong(0L);
 
-  public static void reset() {
-    ATOMIC_LONG = new AtomicLong(0L);
-  }
-
-  public static void writeTestData(
+  public static List<Long> writeTestData(
       ShuffleWriteHandler writeHandler,
       int num, int length,
       Map<Long, byte[]> expectedData,
       Set<Long> expectedBlockIds) throws Exception {
     List<ShufflePartitionedBlock> blocks = Lists.newArrayList();
+    List<Long> blockIds = Lists.newArrayList();

Review Comment:
   Let's use `LinkedHashSet` for `expectedBlockIds`?
   
   The signature could retain or just changed to `LinkedHashSet<Long>`.  This is the test method, we can change it at will.



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

[GitHub] [incubator-uniffle] kaijchen commented on pull request #478: [Test] Assume unknown blockID in LocalFileHandlerTestBase

Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #478:
URL: https://github.com/apache/incubator-uniffle/pull/478#issuecomment-1384050441

   Thanks @advancedxy and @zuston for the review.


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

[GitHub] [incubator-uniffle] advancedxy merged pull request #478: [Test] Assume unknown blockID in LocalFileHandlerTestBase

Posted by GitBox <gi...@apache.org>.
advancedxy merged PR #478:
URL: https://github.com/apache/incubator-uniffle/pull/478


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

[GitHub] [incubator-uniffle] zuston commented on pull request #478: [Test] Make LocalFileHandlerTestBase stateless

Posted by GitBox <gi...@apache.org>.
zuston commented on PR #478:
URL: https://github.com/apache/incubator-uniffle/pull/478#issuecomment-1381250892

   > Not all blocks in blockIdToData are copied in LocalFileHandlerTestBase#calcSegmentBytes().
   
   What about passing the correct wroteBlockList


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

[GitHub] [incubator-uniffle] kaijchen commented on pull request #478: [Test] Make LocalFileHandlerTestBase stateless

Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #478:
URL: https://github.com/apache/incubator-uniffle/pull/478#issuecomment-1380592875

   > or just retrieve blocks from `blockIdToData`
   
   Not all blocks in `blockIdToData` are copied in `LocalFileHandlerTestBase#calcSegmentBytes()`.


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

[GitHub] [incubator-uniffle] codecov-commenter commented on pull request #478: [Test] Make LocalFileHandlerTestBase stateless

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #478:
URL: https://github.com/apache/incubator-uniffle/pull/478#issuecomment-1380292406

   # [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/478?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#478](https://codecov.io/gh/apache/incubator-uniffle/pull/478?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9d29e78) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/7f89d6f61b525d18095d882662c8ef684e2f913f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7f89d6f) will **decrease** coverage by `0.65%`.
   > The diff coverage is `n/a`.
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #478      +/-   ##
   ============================================
   - Coverage     58.78%   58.12%   -0.66%     
   + Complexity     1704     1484     -220     
   ============================================
     Files           206      184      -22     
     Lines         11471     9634    -1837     
     Branches       1024      864     -160     
   ============================================
   - Hits           6743     5600    -1143     
   + Misses         4317     3666     -651     
   + Partials        411      368      -43     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/478?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...storage/handler/impl/DataSkippableReadHandler.java](https://codecov.io/gh/apache/incubator-uniffle/pull/478?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmFnZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvdW5pZmZsZS9zdG9yYWdlL2hhbmRsZXIvaW1wbC9EYXRhU2tpcHBhYmxlUmVhZEhhbmRsZXIuamF2YQ==) | `83.78% <0.00%> (-2.71%)` | :arrow_down: |
   | [...apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java](https://codecov.io/gh/apache/incubator-uniffle/pull/478?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL3YyL2FwcC9Sc3NNUkFwcE1hc3Rlci5qYXZh) | | |
   | [.../java/org/apache/spark/shuffle/RssSparkConfig.java](https://codecov.io/gh/apache/incubator-uniffle/pull/478?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS9Sc3NTcGFya0NvbmZpZy5qYXZh) | | |
   | [...che/spark/shuffle/writer/BufferManagerOptions.java](https://codecov.io/gh/apache/incubator-uniffle/pull/478?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS93cml0ZXIvQnVmZmVyTWFuYWdlck9wdGlvbnMuamF2YQ==) | | |
   | [...preduce/task/reduce/RssRemoteMergeManagerImpl.java](https://codecov.io/gh/apache/incubator-uniffle/pull/478?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL3Rhc2svcmVkdWNlL1Jzc1JlbW90ZU1lcmdlTWFuYWdlckltcGwuamF2YQ==) | | |
   | [.../hadoop/mapreduce/task/reduce/RssEventFetcher.java](https://codecov.io/gh/apache/incubator-uniffle/pull/478?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL3Rhc2svcmVkdWNlL1Jzc0V2ZW50RmV0Y2hlci5qYXZh) | | |
   | [...k/shuffle/writer/WrappedByteArrayOutputStream.java](https://codecov.io/gh/apache/incubator-uniffle/pull/478?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS93cml0ZXIvV3JhcHBlZEJ5dGVBcnJheU91dHB1dFN0cmVhbS5qYXZh) | | |
   | [...rg/apache/hadoop/mapred/RssMapOutputCollector.java](https://codecov.io/gh/apache/incubator-uniffle/pull/478?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkL1Jzc01hcE91dHB1dENvbGxlY3Rvci5qYXZh) | | |
   | [...pache/spark/shuffle/writer/WriteBufferManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/478?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS93cml0ZXIvV3JpdGVCdWZmZXJNYW5hZ2VyLmphdmE=) | | |
   | [...mapreduce/task/reduce/RssInMemoryRemoteMerger.java](https://codecov.io/gh/apache/incubator-uniffle/pull/478?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL3Rhc2svcmVkdWNlL1Jzc0luTWVtb3J5UmVtb3RlTWVyZ2VyLmphdmE=) | | |
   | ... and [13 more](https://codecov.io/gh/apache/incubator-uniffle/pull/478?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


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

[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #478: [Test] Assume unknown blockID in LocalFileHandlerTestBase

Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #478:
URL: https://github.com/apache/incubator-uniffle/pull/478#discussion_r1070985328


##########
storage/src/test/java/org/apache/uniffle/storage/handler/impl/LocalFileHandlerTestBase.java:
##########
@@ -44,26 +44,23 @@
 public class LocalFileHandlerTestBase {
   private static AtomicLong ATOMIC_LONG = new AtomicLong(0L);
 
-  public static void reset() {
-    ATOMIC_LONG = new AtomicLong(0L);
-  }
-
-  public static void writeTestData(
-      ShuffleWriteHandler writeHandler,
-      int num, int length,
-      Map<Long, byte[]> expectedData,
-      Set<Long> expectedBlockIds) throws Exception {
+  public static List<ShufflePartitionedBlock> generateBlocks(int num, int length) {
     List<ShufflePartitionedBlock> blocks = Lists.newArrayList();
     for (int i = 0; i < num; i++) {
       byte[] buf = new byte[length];
       new Random().nextBytes(buf);
       long blockId = ATOMIC_LONG.incrementAndGet();
       blocks.add(new ShufflePartitionedBlock(length, length, ChecksumUtils.getCrc32(buf), blockId, 100,
           buf));
-      expectedData.put(blockId, buf);
-      expectedBlockIds.add(blockId);
     }
-    writeHandler.write(blocks);
+    return blocks;
+  }
+
+  public static void writeTestData(List<ShufflePartitionedBlock> blocks, ShuffleWriteHandler handler,
+      Map<Long, byte[]> expectedData, Set<Long> expectedBlockIds) throws Exception {
+    handler.write(blocks);
+    blocks.forEach(block -> expectedBlockIds.add(block.getBlockId()));
+    blocks.forEach(block -> expectedData.put(block.getBlockId(), block.getData()));

Review Comment:
   Why not do above two operations in one `foreach` ? 



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

[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #478: [Test] Assume unknown blockID in LocalFileHandlerTestBase

Posted by GitBox <gi...@apache.org>.
kaijchen commented on code in PR #478:
URL: https://github.com/apache/incubator-uniffle/pull/478#discussion_r1070989639


##########
storage/src/test/java/org/apache/uniffle/storage/handler/impl/LocalFileHandlerTestBase.java:
##########
@@ -44,26 +44,23 @@
 public class LocalFileHandlerTestBase {
   private static AtomicLong ATOMIC_LONG = new AtomicLong(0L);
 
-  public static void reset() {
-    ATOMIC_LONG = new AtomicLong(0L);
-  }
-
-  public static void writeTestData(
-      ShuffleWriteHandler writeHandler,
-      int num, int length,
-      Map<Long, byte[]> expectedData,
-      Set<Long> expectedBlockIds) throws Exception {
+  public static List<ShufflePartitionedBlock> generateBlocks(int num, int length) {
     List<ShufflePartitionedBlock> blocks = Lists.newArrayList();
     for (int i = 0; i < num; i++) {
       byte[] buf = new byte[length];
       new Random().nextBytes(buf);
       long blockId = ATOMIC_LONG.incrementAndGet();
       blocks.add(new ShufflePartitionedBlock(length, length, ChecksumUtils.getCrc32(buf), blockId, 100,
           buf));
-      expectedData.put(blockId, buf);
-      expectedBlockIds.add(blockId);
     }
-    writeHandler.write(blocks);
+    return blocks;
+  }
+
+  public static void writeTestData(List<ShufflePartitionedBlock> blocks, ShuffleWriteHandler handler,
+      Map<Long, byte[]> expectedData, Set<Long> expectedBlockIds) throws Exception {
+    handler.write(blocks);
+    blocks.forEach(block -> expectedBlockIds.add(block.getBlockId()));
+    blocks.forEach(block -> expectedData.put(block.getBlockId(), block.getData()));

Review Comment:
   There is no significant peformance difference. And I prefer do one thing at a time for clarity.



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

[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #478: [Test] Assume unknown blockID in LocalFileHandlerTestBase

Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #478:
URL: https://github.com/apache/incubator-uniffle/pull/478#discussion_r1068954836


##########
storage/src/test/java/org/apache/uniffle/storage/handler/impl/LocalFileHandlerTestBase.java:
##########
@@ -44,16 +44,13 @@
 public class LocalFileHandlerTestBase {
   private static AtomicLong ATOMIC_LONG = new AtomicLong(0L);
 
-  public static void reset() {
-    ATOMIC_LONG = new AtomicLong(0L);
-  }
-
-  public static void writeTestData(
+  public static List<Long> writeTestData(
       ShuffleWriteHandler writeHandler,
       int num, int length,
       Map<Long, byte[]> expectedData,
       Set<Long> expectedBlockIds) throws Exception {
     List<ShufflePartitionedBlock> blocks = Lists.newArrayList();
+    List<Long> blockIds = Lists.newArrayList();

Review Comment:
   Why not use `Set<Long> expectedBlockIds` directly? 



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

[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #478: [Test] Assume unknown blockID in LocalFileHandlerTestBase

Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #478:
URL: https://github.com/apache/incubator-uniffle/pull/478#discussion_r1070764294


##########
storage/src/test/java/org/apache/uniffle/storage/handler/impl/LocalFileHandlerTestBase.java:
##########
@@ -44,16 +44,13 @@
 public class LocalFileHandlerTestBase {
   private static AtomicLong ATOMIC_LONG = new AtomicLong(0L);
 
-  public static void reset() {
-    ATOMIC_LONG = new AtomicLong(0L);
-  }
-
-  public static void writeTestData(
+  public static List<Long> writeTestData(
       ShuffleWriteHandler writeHandler,
       int num, int length,
       Map<Long, byte[]> expectedData,
       Set<Long> expectedBlockIds) throws Exception {
     List<ShufflePartitionedBlock> blocks = Lists.newArrayList();
+    List<Long> blockIds = Lists.newArrayList();

Review Comment:
   Like this? 
   ![image](https://user-images.githubusercontent.com/8609142/212585816-27817581-87fb-4ae2-af18-b46fcc71ccc7.png)
   



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

[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #478: [Test] Assume unknown blockID in LocalFileHandlerTestBase

Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #478:
URL: https://github.com/apache/incubator-uniffle/pull/478#discussion_r1069093829


##########
storage/src/test/java/org/apache/uniffle/storage/handler/impl/LocalFileServerReadHandlerTest.java:
##########
@@ -80,7 +78,7 @@ public void testDataInconsistent() throws Exception {
     int readBufferSize = 13;
     int bytesPerSegment = ((readBufferSize / blockSize) + 1) * blockSize;
     List<byte[]> segments = LocalFileHandlerTestBase.calcSegmentBytes(expectedData,
-        bytesPerSegment, actualWriteDataBlock);
+        bytesPerSegment, blockIds.subList(0, actualWriteDataBlock));

Review Comment:
   The size of expectedBlockIds is expected in this method of `calcSegmentBytes` invoking. Right? 



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