Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wankunde
Copy link

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

Copy link

github-actions bot commented Jul 21, 2024

Test Results

 2 717 files  +32   2 717 suites  +32   5h 35m 51s ⏱️ + 12m 0s
   968 tests ± 0     967 ✅ + 2   1 💤 ±0  0 ❌  - 2 
12 129 runs  +61  12 114 ✅ +63  15 💤 ±0  0 ❌  - 2 

Results for commit 0687cee. ± Comparison against base commit b5cac30.

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

♻️ This comment has been updated with latest results.

@wankunde wankunde changed the title [WIP][#1938] improvement: Use ShuffleSegment to replace FileBasedShuffleSegment and BufferSegment [#1938] improvement: Use ShuffleSegment to replace FileBasedShuffleSegment and BufferSegment Jul 22, 2024
@zuston zuston requested a review from leixm August 19, 2024 02:45
@zuston
Copy link
Member

zuston commented Aug 19, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants