HDFS-17606. Do not require implementing CustomizedCallbackHandler. #7005
Conversation
This file contains 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
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| @@ -34,6 +36,24 @@ public void handleCallback(List<Callback> callbacks, String username, char[] pas | |||
| } | |||
| } | |||
|
|
|||
| static CustomizedCallbackHandler delegate(Object delegated) { | |||
There was a problem hiding this comment.
As of last week, DynMethod exists in hadoop common to assist here. Have look to see if it would help or are your needs better. A key aspect is can extract IOEs from the invocation
There was a problem hiding this comment.
@steveloughran , thanks for the info! Since we need to back port this to some earlier versions, we won't use DynMethod at the moment.
steveloughran
requested changes
Aug 19, 2024
|
|
||
| // without setting conf, expect UnsupportedCallbackException | ||
| try { | ||
| new SaslServerCallbackHandler(conf, String::toCharArray).handle(callbacks); |
There was a problem hiding this comment.
use LambdaTestUtils.intercept()
| // set conf and expect success | ||
| conf.setClass(HdfsClientConfigKeys.DFS_DATA_TRANSFER_SASL_CUSTOMIZEDCALLBACKHANDLER_CLASS_KEY, | ||
| MyCallbackMethod.class, Object.class); | ||
| new SaslServerCallbackHandler(conf, String::toCharArray).handle(callbacks); |
There was a problem hiding this comment.
add a static counter somewhere to verify the callback was actually invoked.
add another test to raise an exception in the callback to verify it gets reported.
There was a problem hiding this comment.
Sure, will add both.
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
steveloughran
approved these changes
Aug 20, 2024
There was a problem hiding this comment.
+1 once you add the javadoc and are confident the failure is unrelated
the assertj recommendations are hints, it's just time you embraced assertJ for its powerful comparators, especially on sets, lists etc.
I had a look to see if java functional APIs had an interface you could just probe for/cast to instead, but sadly there isn't. If there was there'd be no need for reflection at all
| LAST_CALLBACKS.set(callbacks); | ||
| } | ||
|
|
||
| static void assertCallbacks(Callback[] expected) { |
There was a problem hiding this comment.
add a javadoc explaining what it does
|
|
||
| static void assertCallbacks(Callback[] expected) { | ||
| final List<Callback> computed = LAST_CALLBACKS.getAndSet(null); | ||
| Assert.assertNotNull(computed); |
There was a problem hiding this comment.
I think you could do this in a single AssertJ assertion, especially lines 49-51, provides expected was actually a list.
AssertJ.assertThat(computed)
.describedAs("computed callbacks")
.isNotNull()
.hasSameElementsAs(expected)There was a problem hiding this comment.
Tried it but AssertJ was not available for import. So I will keep the current code and avoid adding the dependency.
The failure of TestMover is unrelated to this. Let's retry. |
|
🎊 +1 overall
This message was automatically generated. |
|
@steveloughran , thanks a lots for reviewing this! |
KeeProMise
pushed a commit
to KeeProMise/hadoop
that referenced
this pull request
Sep 9, 2024
Hexiaoqiao
pushed a commit
to Hexiaoqiao/hadoop
that referenced
this pull request
Sep 12, 2024
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.
Description of PR
HDFS-17576 added a
CustomizedCallbackHandlerinterface which declares the following method:This Jira HDFS-17606 is to allow an implementation to define the handleCallback method without implementing the
CustomizedCallbackHandlerinterface. It is to avoid a security provider depending on the HDFS project.How was this patch tested?
A new test.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?