-
Notifications
You must be signed in to change notification settings - Fork 170
Introduce the extraProperties to support user-defined pluggable accessCheckers #42
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
Conversation
|
Could you give us some examples? |
|
Could u help check this PR? @duanmeng |
aa42109 to
a028f75
Compare
Codecov Report
@@ Coverage Diff @@
## master #42 +/- ##
============================================
- Coverage 55.18% 55.12% -0.06%
+ Complexity 1112 1110 -2
============================================
Files 149 149
Lines 7964 7969 +5
Branches 760 761 +1
============================================
- Hits 4395 4393 -2
- Misses 3328 3333 +5
- Partials 241 243 +2
Continue to review full report at Codecov.
|
|
Is this a compatible feature? Can low version client use high version server? Can high version client use low version server? |
|
I think it's an incompatible change. |
I think we can make it become a compatible feature. Because protobuffer have backwards-compatibility. |
|
@jerqi |
coordinator/src/main/java/org/apache/uniffle/coordinator/CoordinatorGrpcService.java
Outdated
Show resolved
Hide resolved
|
LGTM, let @duanmeng take a look and decide whether to approve. |
internal-client/src/main/java/org/apache/uniffle/client/request/RssAccessClusterRequest.java
Outdated
Show resolved
Hide resolved
coordinator/src/main/java/org/apache/uniffle/coordinator/CoordinatorGrpcService.java
Outdated
Show resolved
Hide resolved
f3be39f to
346c6d1
Compare
duanmeng
left a comment
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
What changes were proposed in this pull request?
Introduce the reservedData to extend more pluggable accessCheckers
Why are the changes needed?
In current codebase, the accessinfo only have acessid and tags. If we want to extend more AccessChecker in coordinator, the info is not enough.
To solve this, i think introducing the reservedData is necessary.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
UTs