HBASE-22377 Provide API to check the existence of a namespace which does not require ADMIN permissions #225
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reidddddd
reviewed
May 8, 2019
| import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProtos.TruncateTableResponse; | ||
| import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProtos.UnassignRegionRequest; | ||
| import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProtos.UnassignRegionResponse; | ||
| import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProtos.*; |
Contributor
Author
There was a problem hiding this comment.
Yeah Eclipse did this behind my back. Will manually undo it.
Reidddddd
reviewed
May 8, 2019
| import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProtos.TruncateTableResponse; | ||
| import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProtos.UnassignRegionRequest; | ||
| import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProtos.UnassignRegionResponse; | ||
| import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProtos.*; |
Contributor
|
LGTM overall, I just left two comments about import check-style. |
the-sakthi
reviewed
May 8, 2019
| public void preListNamespaces(final List<String> namespaces) throws IOException { | ||
| execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() { | ||
| @Override | ||
| public void call(MasterObserver oserver) throws IOException { |
Member
There was a problem hiding this comment.
Just a minor spelling nit:
Suggested change
| public void call(MasterObserver oserver) throws IOException { | |
| public void call(MasterObserver observer) throws IOException { |
the-sakthi
reviewed
May 8, 2019
| execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() { | ||
| @Override | ||
| public void call(MasterObserver oserver) throws IOException { | ||
| oserver.preListNamespaces(this, namespaces); |
Member
There was a problem hiding this comment.
Suggested change
| oserver.preListNamespaces(this, namespaces); | |
| observer.preListNamespaces(this, namespaces); |
the-sakthi
reviewed
May 8, 2019
| public void postListNamespaces(final List<String> namespaces) throws IOException { | ||
| execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() { | ||
| @Override | ||
| public void call(MasterObserver oserver) throws IOException { |
Member
There was a problem hiding this comment.
Suggested change
| public void call(MasterObserver oserver) throws IOException { | |
| public void call(MasterObserver observer) throws IOException { |
the-sakthi
reviewed
May 8, 2019
| execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() { | ||
| @Override | ||
| public void call(MasterObserver oserver) throws IOException { | ||
| oserver.postListNamespaces(this, namespaces); |
Member
There was a problem hiding this comment.
Suggested change
| oserver.postListNamespaces(this, namespaces); | |
| observer.postListNamespaces(this, namespaces); |
the-sakthi
reviewed
May 8, 2019
Member
the-sakthi
left a comment
the-sakthi
left a comment
There was a problem hiding this comment.
Looks good to me. Just left a minor nit.
Member
|
Also, would it make sense to add a comment somewhere in master or preListNamespaces or even the client side about making sure no accesscontroller check is skipped on purpose so that folks who try to introduce that change are aware of the the requirement of this issue? And how about putting in few tests cases around admin.listNamespaces() that checks this as well in the future as there aren't very many tests hitting the admin.listNamespaces codepath currently? |
Contributor
Author
Good suggestion
I can certainly add one, no problem. Also, over on the JIRA there is a TestInterfaceAlign result that tells me I missed an update to AsyncAdmin so let me add that too. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
First force push above was just a rebase; second (latest) push is the update. Changes:
|
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
Updated patch to fix checkstyle nits |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
The rubocop result is silly. I left the formatting the same as I found it. Somehow it was not valid before? But if someone feels this is legitimate, I can just drop the change to admin.rb, it is just a trivial thing. The unit test failure looks unrelated, perhaps environmental. |
Reidddddd
reviewed
May 13, 2019
| list = @admin.listNamespaceDescriptors.map(&:getName) | ||
| list.select { |s| pattern.match(s) } | ||
| list = @admin.listNamespaces | ||
| list.select {|s| pattern.match(s) } |
Contributor
There was a problem hiding this comment.
I think the warning pointing here {|s| which should be { |s|
Contributor
Author
There was a problem hiding this comment.
Thank you. I was blind to that whitespace change.
Contributor
Author
There was a problem hiding this comment.
Actually I didn't change that line intentionally, so that was my confusion. Will update it...
Contributor
|
Confirmed, the failed test is unrelated. |
Contributor
Author
|
Updated patch with admin.rb whitespace fix |
xcangCRM
approved these changes
May 13, 2019
Contributor
xcangCRM
left a comment
xcangCRM
left a comment
There was a problem hiding this comment.
+1
and just 2 nitpickings.
| * List available namespace descriptors. | ||
| * List available namespaces | ||
| * | ||
| * @return List of descriptors |
Contributor
There was a problem hiding this comment.
Nit: also change this to "List of namespace names", not descriptors anymore
Contributor
Author
There was a problem hiding this comment.
Will fix on commit, thanks
| String[] res = new String[list.size()]; | ||
| for(int i = 0; i < list.size(); i++) { | ||
| res[i] = list.get(i); | ||
| } |
Contributor
There was a problem hiding this comment.
Nit:
should we just use toArray to convert a list to an array, removing the for loop?
res = list.toArray(res);
Contributor
Author
There was a problem hiding this comment.
Ok, will fix on commit
…oes not require ADMIN permissions
|
💔 -1 overall
This message was automatically generated. |
asfgit
pushed a commit
that referenced
this pull request
May 14, 2019
…oes not require ADMIN permissions (#225) Signed-off-by: Xu Cang <xucang@apache.org>
asfgit
pushed a commit
that referenced
this pull request
May 14, 2019
…oes not require ADMIN permissions (#225) Signed-off-by: Xu Cang <xucang@apache.org>
infraio
pushed a commit
to infraio/hbase
that referenced
this pull request
Aug 17, 2020
…oes not require ADMIN permissions (apache#225) Signed-off-by: Xu Cang <xucang@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.