HDFS-17793. [ARR] RBF: Enable the router asynchronous RPC feature to handle DelegationToken request errors #7714
+40
−0
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
… getDelegationToken request errors
|
Hi, @zhangxiping1 . Thanks for reporting this problem. Make sense to me. |
@Override
public Token<DelegationTokenIdentifier> getDelegationToken(Text renewer)
throws IOException {
rpcServer.checkOperation(NameNode.OperationCategory.WRITE, true);
asyncComplete(this.securityManager.getDelegationToken(renewer));
return asyncReturn(Token.class);
}
@Override
public long renewDelegationToken(Token<DelegationTokenIdentifier> token)
throws IOException {
rpcServer.checkOperation(NameNode.OperationCategory.WRITE, true);
asyncComplete(this.securityManager.renewDelegationToken(token));
return asyncReturn(Long.class);
}
@Override
public void cancelDelegationToken(Token<DelegationTokenIdentifier> token)
throws IOException {
rpcServer.checkOperation(NameNode.OperationCategory.WRITE, true);
this.securityManager.cancelDelegationToken(token);
asyncComplete(null);
} |
KeeProMise
changed the title
KeeProMise
changed the title
@KeeProMise ok，I'll revise and submit the following later. |
There was a problem hiding this comment.
Pull Request Overview
Enable the router’s asynchronous RPC support for delegation token operations by removing the old synchronous overrides and adding a basic test.
- Added a test stub for
getDelegationTokenin the async RPC test suite. - Removed the synchronous
getDelegationToken,renewDelegationToken, andcancelDelegationTokenoverrides in favor of the async router mechanism.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hadoop-hdfs-project/.../TestRouterAsyncRpc.java | Added testgetDelegationToken stub without assertions |
| hadoop-hdfs-project/.../RouterClientNamenodeProtocolServerSideTranslatorPB.java | Removed synchronous delegation-token RPC overrides |
Comments suppressed due to low confidence (2)
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/async/TestRouterAsyncRpc.java:95
- [nitpick] The method name
testgetDelegationTokenis inconsistent with camelCase conventions; consider renaming it totestGetDelegationTokenfor readability and consistency.
public void testgetDelegationToken() throws Exception {
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/protocolPB/RouterClientNamenodeProtocolServerSideTranslatorPB.java:979
- The synchronous overrides for delegation token operations were removed without replacement; callers of these RPCs will now receive
null. Implement the asyncRouterServer logic forgetDelegationToken,renewDelegationToken, andcancelDelegationTokento ensure proper handling.
@Override
|
|
||
| @Test | ||
| public void testgetDelegationToken() throws Exception { | ||
| rndRouter.getFileSystem().getDelegationToken("yarn"); |
There was a problem hiding this comment.
This test does not include any assertions to verify the returned token or expected error; add assertions (e.g., assertNotNull or expected exception) to validate behavior.
Suggested change
| rndRouter.getFileSystem().getDelegationToken("yarn"); | |
| // Get the delegation token for the user "yarn". | |
| org.apache.hadoop.security.token.Token<?> token = | |
| rndRouter.getFileSystem().getDelegationToken("yarn"); | |
| // Assert that the token is not null. | |
| assertNotNull(token, "Delegation token should not be null"); | |
| // Optionally, verify additional properties of the token. | |
| assertEquals("yarn", token.getService().toString(), | |
| "The token service should match the requested user"); |
Copilot uses AI. Check for mistakes.
|
🎊 +1 overall
This message was automatically generated. |
…handle DelegationToken request errors
… to handle DelegationToken request errors
|
🎊 +1 overall
This message was automatically generated. |
|
@zhangxiping1 Here is a Checkstyle exception /hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterClientProtocol.java:171: protected RouterSecurityManager securityManager = null;:35: Variable 'securityManager' must be private and have accessor methods. [VisibilityModifier] |
ok ，I'll make some more revisions. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@KeeProMise hi, lao tie, it seems to be okay. |
@zhangxiping1 LGTM + 1. Let's wait for two days to see if there are any other comments. |
KeeProMise
reviewed
May 28, 2025
Comment on lines
93
to
94
|
|
||
|
|
There was a problem hiding this comment.
Hi @zhangxiping1 Just leave a blank line here.
|
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Pull Request Overview
This PR enables the router’s asynchronous RPC feature to support delegation token operations and adds a basic test for the new async getDelegationToken call.
- Added async implementations for
getDelegationToken,renewDelegationToken, andcancelDelegationTokeninRouterAsyncClientProtocol - Exposed
getSecurityManager()inRouterClientProtocolto back the async handlers - Introduced a new test to verify no exception is thrown when calling
getDelegationTokenasynchronously
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| TestRouterAsyncRpc.java | Added testGetDelegationTokenAsyncRpc using assertDoesNotThrow |
| RouterAsyncClientProtocol.java | Implemented async methods for delegation token operations |
| RouterClientProtocol.java | Exposed getSecurityManager() getter |
Comments suppressed due to low confidence (2)
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/async/TestRouterAsyncRpc.java:97
- The test only asserts that no exception is thrown; consider adding
assertNotNullon the returned token to validate the async call actually returns a valid delegation token.
assertDoesNotThrow(() -> {
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/async/TestRouterAsyncRpc.java:94
- There are no tests covering
renewDelegationTokenorcancelDelegationTokenasync methods; consider adding similar tests to ensure those flows handle errors correctly.
@Test
| throws IOException { | ||
| rpcServer.checkOperation(NameNode.OperationCategory.WRITE, true); | ||
| asyncComplete(getSecurityManager().getDelegationToken(renewer)); | ||
| return asyncReturn(Token.class); |
There was a problem hiding this comment.
[nitpick] Using asyncReturn(Token.class) loses the generic type information (DelegationTokenIdentifier). Consider an explicit cast or adding @SuppressWarnings("unchecked") to make the conversion clear and safe.
Suggested change
| return asyncReturn(Token.class); | |
| return (Token<DelegationTokenIdentifier>) asyncReturn(Token.class); |
Copilot uses AI. Check for mistakes.
| @@ -2524,4 +2524,8 @@ public void setServerDefaultsLastUpdate(long serverDefaultsLastUpdate) { | |||
| public RouterFederationRename getRbfRename() { | |||
| return rbfRename; | |||
| } | |||
|
|
|||
| public RouterSecurityManager getSecurityManager() { | |||
There was a problem hiding this comment.
This new public getter lacks Javadoc and audience annotations; consider adding a brief description and an @InterfaceAudience tag to clarify intended use.
Copilot uses AI. Check for mistakes.
KeeProMise
approved these changes
May 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Enable the router asynchronous RPC feature to handle getDelegationToken request errors.