-
Notifications
You must be signed in to change notification settings - Fork 141
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
[#477][part-0] feat: add ShuffleManagerServer impl #777
Conversation
Codecov Report
@@ Coverage Diff @@
## master #777 +/- ##
============================================
- Coverage 61.16% 59.32% -1.84%
- Complexity 1985 2029 +44
============================================
Files 245 284 +39
Lines 13422 12554 -868
Branches 1125 1185 +60
============================================
- Hits 8209 7448 -761
+ Misses 4751 4693 -58
+ Partials 462 413 -49
... and 68 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
eb8d61f
to
67fb1c8
Compare
common/src/main/java/org/apache/uniffle/common/metrics/DummyGRPCMetrics.java
Outdated
Show resolved
Hide resolved
...st/spark-common/src/test/java/org/apache/uniffle/test/SimpleShuffleServerManagerE2ETest.java
Outdated
Show resolved
Hide resolved
...t/src/main/java/org/apache/uniffle/client/response/RssReportShuffleFetchFailureResponse.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@zuston do you some any more comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Left minor comments
...spark/common/src/main/java/org/apache/uniffle/shuffle/manager/ShuffleManagerGrpcService.java
Show resolved
Hide resolved
...ark/common/src/main/java/org/apache/uniffle/shuffle/manager/ShuffleManagerServerFactory.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
public GrpcServer getServer() { | ||
String type = conf.getString(RssBaseConf.RPC_SERVER_TYPE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks RPC_SERVER_TYPE
should be refactored as ENUM
type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are multiple definitions of ServerType enums, let's refactor that in a new pr.
...rnal-client/src/main/java/org/apache/uniffle/client/factory/ShuffleManagerClientFactory.java
Outdated
Show resolved
Hide resolved
public class ShuffleManagerGrpcClient extends GrpcClient implements ShuffleManagerClient { | ||
|
||
private static final Logger LOG = LoggerFactory.getLogger(ShuffleManagerGrpcClient.class); | ||
private static final long RPC_TIMEOUT_DEFAULT_MS = 60000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be. Let's do this in another pr. This code is aligned with other Client impls, such as ShuffleServerGrpcClient.
We should refactor these together.
### What changes were proposed in this pull request? 1. add shuffle manager proto 2. the corresponding client and server impls 3. add the RssShuffleManagerInterface ### Why are the changes needed? This is the first part of apache#477. To support re-submit spark stage, the ShuffleManagerServer should be introduced first. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added UTs and one integration test.
What changes were proposed in this pull request?
Why are the changes needed?
This is the first part of #477.
To support re-submit spark stage, the ShuffleManagerServer should be introduced first.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Added UTs and one integration test.