Uh oh!
There was an error while loading. Please reload this page.
HBase-22027: Split non-MR related parts of TokenUtil off into a Clien… - #361
Conversation
…tTokenUtil, and move ClientTokenUtil to hbase-client
| }); | ||
| return future; | ||
| } | ||
This comment has been minimized.
This comment has been minimized.
| package org.apache.hadoop.hbase.security.token; | ||
| import com.google.protobuf.ByteString; |
There was a problem hiding this comment.
Why are we using the non-relocated classes here?
There was a problem hiding this comment.
This is a mistake, nice catch. Will fix.
There was a problem hiding this comment.
The deleted code in TokenUtil was using non relocated classes, as AuthenticationProtos uses the unshaded classes. Should I update AuthenticationProtos to reference shaded classes, or what do you think I should do here?
There was a problem hiding this comment.
Which I guess would require figuring out how to modify the generated code.
There was a problem hiding this comment.
I've left the imports as-is for now.
There was a problem hiding this comment.
I think thats right for this client-facing class. Client stuff is all unshaded 2.5 pb hbase-protocol (as opposed to hbase-protocol-shaded).
There was a problem hiding this comment.
Having second thoughts now.
Why can't this new class be an Interface?
If an Interface, can hide stuff like this.
I can help refactor if you think Interface will work. Thanks
There was a problem hiding this comment.
I'm not sure I follow. How does making ClientTokenUtil an interface affect class shading? The implementation of the ClientTokenUtil interface would still have to reference the unshaded classes, right?
If you have an idea for refactoring this, I'd be happy to merge it into this PR. If you put up a PR against the HBASE-22027 branch at https://github.com/srdo/hbase, I could update this PR with your changes.
There was a problem hiding this comment.
I tried my 'idea' and realize my suggestion of no help -- pardon me. Trying my idea did help me w/ this review though.
Yes, it is appropriate here to use the non-relocated protobuf stuff --i.e. as you have it -- while auth goes via Coprocessor API.
| * @throws IOException if a remote error or serialization problem occurs. | ||
| * @return the authentication token instance | ||
| * See {@link ClientTokenUtil#obtainToken(org.apache.hadoop.hbase.client.Connection)}. | ||
| * @deprecated Please use the corresponding method in {@link ClientTokenUtil} instead. |
There was a problem hiding this comment.
This and several of the other deprecated methods point folks to a method that's labeled IA.Private.
If there isn't a IA.Public method to send folks to, we should call out in the deprecation that folks should stop using the functionality or come ask dev@ if it's needed
There was a problem hiding this comment.
I'm going by the posts here https://issues.apache.org/jira/browse/HBASE-22027?focusedCommentId=16789499&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16789499. As I understand, these methods should be IA.Private, as they return types that are IA.Private.
I don't know if there's a public replacement, but I'm happy to update the deprecation notice.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
srdo
commented
Jul 15, 2019
The failure on Jenkins seems to be the hbase-server not compiling because it can't find the new ClientTokenUtil class (e.g. https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-361/5/artifact/out/patch-unit-hbase-server.txt). This build command runs fine for me locally. Is the script maybe set up so it is pulling in an old hbase-client jar? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
srdo
commented
Jul 22, 2019
@busbey Is there anything I should do to get this approved? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Apache-HBase
commented
Jul 25, 2019
💔 -1 overall
This message was automatically generated. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Apache-HBase
commented
Sep 2, 2019
💔 -1 overall
This message was automatically generated. |
| package org.apache.hadoop.hbase.security.token; | ||
| import com.google.protobuf.ByteString; |
There was a problem hiding this comment.
I think thats right for this client-facing class. Client stuff is all unshaded 2.5 pb hbase-protocol (as opposed to hbase-protocol-shaded).
Uh oh!
There was an error while loading. Please reload this page.
saintstack
left a comment
There was a problem hiding this comment.
Some notes informed by my trying to make an impl.
| package org.apache.hadoop.hbase.security.token; | ||
| import com.google.protobuf.ByteString; |
There was a problem hiding this comment.
I tried my 'idea' and realize my suggestion of no help -- pardon me. Trying my idea did help me w/ this review though.
Yes, it is appropriate here to use the non-relocated protobuf stuff --i.e. as you have it -- while auth goes via Coprocessor API.
| * @return the authentication token instance, wrapped by a {@link CompletableFuture}. | ||
| */ | ||
| @InterfaceAudience.Private | ||
| public static CompletableFuture<Token<AuthenticationTokenIdentifier>> obtainToken( |
There was a problem hiding this comment.
This is public so the TokenUtil on server can use this method? Can it be package private given both TokenUtil and this class are in the same java package? Then you could drop the IA Private.
There was a problem hiding this comment.
Hmmm... but seems like you want these to be public now? You are pointing users of TokenUtil here?
There was a problem hiding this comment.
Yes, this is public so TokenUtil can delegate to this method, but also so any internal HBase code that needs this method can use it. The TokenUtil methods have been deprecated, so new internal HBase code would ideally use ClientTokenUtil instead.
If there isn't a need for these methods elsewhere in HBase, I'm happy to try making them package private.
The Javadoc on the deprecated methods in TokenUtil explicitly says not to use these methods when you are developing non-HBase code. The only users I want to point to ClientTokenUtil (except obtainAndCacheToken) are internal HBase users.
There was a problem hiding this comment.
I'm also happy to update any call sites in HBase so they use the ClientTokenUtil methods instead of the now deprecated TokenUtil methods.
There was a problem hiding this comment.
The audience annotations are for downstreamers, not for hbase internally.
Given ClientTokenUtil is a new class, we have opportunity for setting access as we see fit.
The #toToken methods are used internal to the package only it seems (tests and the class-itself). Suggest these become package private or private especially the overrides that take protobufs.
Otherwise, methods seem innocuous-looking. The less API we make public, the better otherwise, make a call.
Regards changing internal usage, could do here or in a follow-on.
Thanks for persisting on this.
There was a problem hiding this comment.
Only obtainToken(AsyncConnection) seems to need to be public, as it is used by SecureBulkLoadManager. Everything else can be package private. Updated the PR to make the methods package private, as well as to get rid of uses of the deprecated TokenUtil methods.
There was a problem hiding this comment.
Replaced the use of reflection in TestClientTokenUtil with just calling the obtainToken methods directly. The existing code doesn't find package private methods, causing the test to fail. I grepped for the method names, and they don't seem to be used via reflection anywhere else in HBase.
| * @return the authentication token instance | ||
| */ | ||
| @InterfaceAudience.Private | ||
| public static Token<AuthenticationTokenIdentifier> obtainToken( |
There was a problem hiding this comment.
Yeah, looks like used by test only and the tests are in same package.
…hods. Make methods that don't need to be public package-private
Apache-HBase
commented
Sep 17, 2019
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 18, 2019
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 18, 2019
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 19, 2019
🎊 +1 overall
This message was automatically generated. |
saintstack
left a comment
There was a problem hiding this comment.
Very nice.
@busbey Will commit in next day or so. Take a looksee if you get a chance.
| @ClassRule | ||
| public static final HBaseClassTestRule CLASS_RULE = | ||
| HBaseClassTestRule.forClass(TestTokenUtil.class); | ||
| HBaseClassTestRule.forClass(TestClientTokenUtil.class); |
Apache-HBase
commented
Sep 19, 2019
🎊 +1 overall
This message was automatically generated. |
apache#361) * HBase-22027: Split non-MR related parts of TokenUtil off into a ClientTokenUtil, and move ClientTokenUtil to hbase-client * Replace uses of deprecated TokenUtil methods with ClientTokenUtil methods. Make methods that don't need to be public package-private * Don't use reflection where not necessary in TestClientTokenUtil Signed-off-by: Sean Busbey <busbey@apache.org> Signed-off-by: stack <stack@apache.org>
…tTokenUtil, and move ClientTokenUtil to hbase-client
See https://issues.apache.org/jira/browse/HBASE-22027