[#1709] feat(coordinator): Introduce pluggable ClientConfApplyManager for fetchClientConf rpc #1710
Conversation
…anager for fetchClientConf rpc
proto/src/main/proto/Rss.proto
Outdated
| @@ -463,6 +464,12 @@ message AccessClusterResponse { | |||
| string uuid = 3; | |||
| } | |||
|
|
|||
| message FetchClientConfRequest { | |||
| string user = 1; | |||
| // for the potential extension for customize delegation shuffle manager | |||
There was a problem hiding this comment.
I will put more internal job properties in internal DelegationShuffleManager, and then extend the internal ClientConfStrategy to apply specific rss conf to job.
There was a problem hiding this comment.
Could you give more comments for this pull request?
| if (exts != null && exts.size() == 1) { | ||
| return exts.get(0); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
What will it happen if we have two extensions?
There was a problem hiding this comment.
Currently, this is illegal
There was a problem hiding this comment.
So maybe we should throw an IllegalException instead of returning null value.
| @@ -351,7 +351,7 @@ public RssFetchClientConfResponse fetchClientConf(RssFetchClientConfRequest requ | |||
| rpcResponse = | |||
| blockingStub | |||
| .withDeadlineAfter(request.getTimeoutMs(), TimeUnit.MILLISECONDS) | |||
| .fetchClientConf(Empty.getDefaultInstance()); | |||
| .fetchClientConfV2(request.toProto()); | |||
There was a problem hiding this comment.
fetchClientConf uses the empty request, it's hard to extend for this PR.
| } | ||
|
|
||
| public boolean isEmpty() { | ||
| return user == null && properties == null; |
There was a problem hiding this comment.
What will it happen if properties is empty?
There was a problem hiding this comment.
It will return the conf directly to be compatible with the previous logic from older client.
There was a problem hiding this comment.
user == null && CollectionUtils.isEmpty(properties)?
There was a problem hiding this comment.
Map could not be checked in CollectionUtils.
There was a problem hiding this comment.
user == null && (properties == null || properties.isEmpty())?
There was a problem hiding this comment.
Map could not be checked in CollectionUtils.
Maybe we can use MapUtils.
There was a problem hiding this comment.
Oh yes. Let's reserve this in this PR.
…Strategy` for `fetchClientConf` rpc (apache#1710) Introduce pluggable ClientConfApplyManager for fetchClientConf rpc For apache#1709 Yes. Unit tests
What changes were proposed in this pull request?
Introduce pluggable ClientConfApplyManager for fetchClientConf rpc
Why are the changes needed?
For #1709
Does this PR introduce any user-facing change?
Yes.
How was this patch tested?
Unit tests