incubator-uniffle icon indicating copy to clipboard operation
incubator-uniffle copied to clipboard

[#1938] improvement: Use ShuffleSegment to replace FileBasedShuffleSegment and BufferSegment

Open wankunde opened this issue 1 year ago • 2 comments

What changes were proposed in this pull request?

Now the BufferSegment class and FileBasedShuffleSegment class both six fields, represents a segment.

  private long blockId;
  private long offset;
  private int length;
  private int uncompressLength;
  private long crc;
  private long taskAttemptId;

To add a new ShuffleSegment to replace these two class can make the code easier to maintain.

Why are the changes needed?

To simplify the code.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Exists UT

wankunde avatar Jul 21 '24 15:07 wankunde

Test Results

 2 717 files  +32   2 717 suites  +32   5h 35m 51s :stopwatch: + 12m 0s    968 tests ± 0     967 :white_check_mark: + 2   1 :zzz: ±0  0 :x:  - 2  12 129 runs  +61  12 114 :white_check_mark: +63  15 :zzz: ±0  0 :x:  - 2 

Results for commit 0687ceeb. ± Comparison against base commit b5cac305.

This pull request removes 9 and adds 9 tests. Note that renamed tests count towards both.
org.apache.uniffle.common.BufferSegmentTest ‑ testEquals
org.apache.uniffle.common.BufferSegmentTest ‑ testGetOffset
org.apache.uniffle.common.BufferSegmentTest ‑ testNotEquals{long, long, int, int, long, long}[1]
org.apache.uniffle.common.BufferSegmentTest ‑ testNotEquals{long, long, int, int, long, long}[2]
org.apache.uniffle.common.BufferSegmentTest ‑ testNotEquals{long, long, int, int, long, long}[3]
org.apache.uniffle.common.BufferSegmentTest ‑ testNotEquals{long, long, int, int, long, long}[4]
org.apache.uniffle.common.BufferSegmentTest ‑ testNotEquals{long, long, int, int, long, long}[5]
org.apache.uniffle.common.BufferSegmentTest ‑ testNotEquals{long, long, int, int, long, long}[6]
org.apache.uniffle.common.BufferSegmentTest ‑ testToString
org.apache.uniffle.common.ShuffleSegmentTest ‑ testEquals
org.apache.uniffle.common.ShuffleSegmentTest ‑ testGetOffset
org.apache.uniffle.common.ShuffleSegmentTest ‑ testNotEquals{long, long, int, int, long, long}[1]
org.apache.uniffle.common.ShuffleSegmentTest ‑ testNotEquals{long, long, int, int, long, long}[2]
org.apache.uniffle.common.ShuffleSegmentTest ‑ testNotEquals{long, long, int, int, long, long}[3]
org.apache.uniffle.common.ShuffleSegmentTest ‑ testNotEquals{long, long, int, int, long, long}[4]
org.apache.uniffle.common.ShuffleSegmentTest ‑ testNotEquals{long, long, int, int, long, long}[5]
org.apache.uniffle.common.ShuffleSegmentTest ‑ testNotEquals{long, long, int, int, long, long}[6]
org.apache.uniffle.common.ShuffleSegmentTest ‑ testToString

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Jul 21 '24 15:07 github-actions[bot]

Sorry for the late reply. @wankunde

Thanks for proposing this. Actually the ShuffleSegment has been introduce to be as the an abstract class. And I think the initial design is to clarify the client/server side shuffleSegment. But I agree with the same part 6 variables should be defined in the existing ShuffleSegment. And there is no need to make the invoking side to use the ShuffleSegment to replace all the Filebased or Buffer segnment.

zuston avatar Aug 19 '24 02:08 zuston