[improve][broker] enhance grant permission module to reduce zk metadata - #16792
[improve][broker] enhance grant permission module to reduce zk metadata#16792TakaHiR07 wants to merge 2 commits into
Conversation
TakaHiR07
changed the title
|
@nodece @Technoboy- @codelipenghui PTAL, this problem would result in permission can not be revoked |
|
Hi @TakaHiR07, Thanks for your contribution! I think you should consider the compatibility here. |
d4584a1 to
1ce5cd6
Compare
|
I have changed the proposal and I think it is the better way to solve this problem and meet the compatibility. Please take a look, @nodece @codelipenghui @BewareMyPower pulsar old version
pulsar new version
|
|
/pulsarbot rerun-failure-checks |
|
This table looks confused, I think you should follow the #16792, you also need to improve the get action. |
1ce5cd6 to
0e5003a
Compare
michaeljmarshall
left a comment
michaeljmarshall
left a comment
There was a problem hiding this comment.
Thanks for your updates @TakaHiR07. I left some comments, let me know what you think.
| } | ||
| String topicUri = topicName.toString(); | ||
| // Enhancement: only grant permission on based topic | ||
| String topicUri = topicName.getPartitionedTopicName(); |
There was a problem hiding this comment.
I think this might have been a misunderstanding of my review. The optimization is simply that we do not recursively add topic names when granting permission in the PersistentTopicsBase#internalGrantPermissionsOnTopic method. I think it is valid to grant a role permission to produce to a single partition of a partitioned topic, so we shouldn't make this update here.
There was a problem hiding this comment.
Good catch! All partitions should have the same permissions, is that right?
| if (numPartitions > 0) { | ||
| for (int i = 0; i < numPartitions; i++) { | ||
| TopicName topicNamePartition = topicName.getPartition(i); | ||
| future = future.thenCompose(unused -> grantPermissionsAsync(topicNamePartition, role, | ||
| actions)); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is all that is required for the optimization (regarding my other comment).
| } | ||
| } | ||
|
|
||
| // If topic is partitioned, add based topic permission |
There was a problem hiding this comment.
I do not think we should grant permission based on the partition's base topic. If the call is about a specific permission, we should respond with the result for that partition.
There was a problem hiding this comment.
@michaeljmarshall Is your idea the same as I described in #16792 (comment) ?? That's what I thought at the beginning, marked as plan 1.
plan 2 is described in #16792 (comment), this one is designed based on the idea of @nodece .
Both of the two plan can enhance. I think plan 1 would have better compatibility with previous version, while plan 2 would have cleaner permission record.
|
@TakaHiR07 hi, I move this PR to release/2.9.5, if you have any questions, please ping me. thanks. |
|
The pr had no activity for 30 days, mark with Stale label. |
|
Since we will start the RC version of
So drag this PR to |
|
@TakaHiR07 - I am so sorry for losing track of this PR. Are you interested in continuing your work on it? If not, I am going to pick up the work in a couple weeks. Thank you for your valuable observations and contributions so far! |
There is another pr about this work, you can take a look of it. #18222. I am glad to continue improve this work |
13 participants
Motivation
related to #16768
Modifications
pulsar old version
pulsar new version
Does this pull request potentially affect one of the following parts:
If
yeswas chosen, please highlight the changesDocumentation
Check the box below or label this PR directly.
Need to update docs?
doc-required(Your PR needs to update docs and you will update later)
doc-not-needed(Please explain why)
doc(Your PR contains doc changes)
doc-complete(Docs have been already added)