HADOOP-19342. SaslRpcServer.AuthMethod print INFO messages in client side #7173
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. |
stoty
reviewed
Nov 19, 2024
| this.code = code; | ||
| this.mechanismName = mechanismName; | ||
| LOG.info("{} {}: code={}, mechanism=\"{}\"", | ||
| LOG.debug("{} {}: code={}, mechanism=\"{}\"", |
There was a problem hiding this comment.
AFAICT this ultimately gets called at roughly the same point during client initialization as previously, it just doesn't show up because the debug level has been reduced to DEBUG from INFO.
There was a problem hiding this comment.
So apart from the log level the only difference is that this will pick up changes in the config
if a new SaslRPCServer is created at a later time ?
There was a problem hiding this comment.
if you aren't using SASL stuff saslMechanismFactory::getMechanism doesn't get invoked.
There was a problem hiding this comment.
... this will pick up changes in the config if a new SaslRPCServer is created at a later time ?
It will load the config only when SaslMechanismFactory.getMechanism is called. So, it won't be loaded with UGI.
|
💔 -1 overall
This message was automatically generated. |
steveloughran
requested changes
Nov 19, 2024
| */ | ||
| package org.apache.hadoop.security; | ||
|
|
||
| import org.apache.hadoop.conf.Configuration; |
There was a problem hiding this comment.
nit: split out org.apache and put in a separate block below
| try (OutputStream out = Files.newOutputStream(Paths.get(coreSite), StandardOpenOption.CREATE)) { | ||
| conf.writeXml(out); | ||
| } | ||
| Configuration.addDefaultResource(coreSite); |
There was a problem hiding this comment.
this is going to contaminate all tests which follow in the same JVM, and make debugging conf problems a lot harder.
what about
- create a new xml conf resource adjacent to core-site.xml
- give it the name of this test
- modify this method so if mechanism == null, the conf option is not set.
- after the test, you could set to being empty XML. (then load a configuration to make sure it still loads)
Configuration conf = new Configuration(false)
if (mechanism != null) conf.set(HADOOP_SECURITY_SASL_MECHANISM_KEY, mechanism);
String newResource = /* work this out */
conf.write(new resource)
Configuration.addDefaultResource(newResource);
and in and @after method, use initConf(null) to reset it.
Avoids overwriting core-site.xml
There was a problem hiding this comment.
You are right that it contaminates all tests.
Tried multiple things but not working very well. One problem is that Configuration has addDefaultResource but not removeDefaultResource. Let skip the test.
| import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.HADOOP_SECURITY_SASL_MECHANISM_KEY; | ||
|
|
||
| /** Test {@link SaslRpcServer.AuthMethod}. */ | ||
| public class TestAuthMethod { |
There was a problem hiding this comment.
extends AbstractHadoopTestBase for timeouts, thread names etc
| this.code = code; | ||
| this.mechanismName = mechanismName; | ||
| LOG.info("{} {}: code={}, mechanism=\"{}\"", | ||
| LOG.debug("{} {}: code={}, mechanism=\"{}\"", |
There was a problem hiding this comment.
if you aren't using SASL stuff saslMechanismFactory::getMechanism doesn't get invoked.
|
💔 -1 overall
This message was automatically generated. |
|
The test failure is related. Looking at it. |
|
Actually, the test failure is not related. Seems the Yetus build has something messed up. Will try a new PR #7174 |
|
Using a new pr #7174 |
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
HADOOP-19342
After HADOOP-19306, the following INFO messages are printed in client side. Thanks As Steve Loughran for reporting it in this comment.
How was this patch tested?
Manual test.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?