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

[MINOR] improvement(spark-client): Refactor RssShuffleManager for spark2 and spark3 to reduce redundant code #2330

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

maobaolong
Copy link
Member

@maobaolong maobaolong commented Jan 7, 2025

What changes were proposed in this pull request?

Refactor and abstract the same code into base class.

Why are the changes needed?

Reduce the redundant code and simplify the development in rss spark-client scope.

The Spark version unrelated code should be placed into RssShuffleManagerBase class, for this RssShuffleManager Class, it should only maintains the spark api related codes.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

  • Existing UTs.
  • Our test cluster, tested both spark v2 and v3 version.

Copy link

github-actions bot commented Jan 7, 2025

Test Results

 2 966 files  +14   2 966 suites  +14   6h 30m 35s ⏱️ + 28m 3s
 1 100 tests ± 0   1 098 ✅ + 1   2 💤 ±0  0 ❌  - 1 
13 774 runs  +24  13 744 ✅ +26  30 💤 ±0  0 ❌  - 2 

Results for commit 5a40312. ± Comparison against base commit 5b7d18c.

♻️ This comment has been updated with latest results.

@maobaolong maobaolong force-pushed the abstractRssShuffleManager branch 2 times, most recently from 127050e to 0fa7b2b Compare January 8, 2025 01:40
@maobaolong
Copy link
Member Author

@jerqi @zuston Would you like to take a look at this PR? Thanks.

@maobaolong maobaolong force-pushed the abstractRssShuffleManager branch 2 times, most recently from 26ff0a5 to 61cb95c Compare January 9, 2025 01:52
@maobaolong maobaolong force-pushed the abstractRssShuffleManager branch from 61cb95c to 5a40312 Compare January 9, 2025 01:58
@maobaolong maobaolong closed this Jan 9, 2025
@maobaolong maobaolong reopened this Jan 9, 2025
@jerqi
Copy link
Contributor

jerqi commented Jan 9, 2025

You are the committer. I can only rerun the failed test case.

@maobaolong
Copy link
Member Author

@jerqi Thanks for your tips.

@maobaolong
Copy link
Member Author

This has been tested on our test cluster for both spark v2 and v3 version client. merge it.

@maobaolong maobaolong merged commit 43879ca into apache:master Jan 9, 2025
80 of 85 checks passed
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