szilard-nemeth commented on code in PR #6027:
URL: https://github.com/apache/hadoop/pull/6027#discussion_r1319234289
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/conf/YarnConfiguration.java:
##########
@@ -853,6 +853,11 @@ public static boolean isAclEnabled(Configuration conf) {
/** Zookeeper interaction configs */
public static final String RM_ZK_PREFIX = RM_PREFIX + "zk-";
+ /** Enable Zookeeper SSL/TLS communication */
+ public static final String RM_ZK_CLIENT_SSL_ENABLED =
+ RM_ZK_PREFIX + "client-ssl.enabled";
+ public static final boolean DEFAULT_RM_ZK_CLIENT_SSL_ENABLED = Boolean.FALSE;
Review Comment:
In this case I don't think it makes too much sense to use the static
Boolean.FALSE object as the type is a primitive boolean anyways.
See:
https://stackoverflow.com/questions/20090306/what-is-the-difference-between-false-and-boolean-false
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestRMStoreCommands.java:
##########
@@ -101,6 +102,16 @@ public void testFormatConfStoreCmdForZK() throws Exception
{
}
}
+ @Test
+ public void testSSLEnabledConfiguration() {
+ //Test if we can enable SSL/TLS for the ZK Curator Client in YARN.
+ Configuration conf = new Configuration();
+ conf.set(YarnConfiguration.RM_ZK_CLIENT_SSL_ENABLED,
Boolean.TRUE.toString());
+
+ assertEquals("The " + YarnConfiguration.RM_ZK_CLIENT_SSL_ENABLED + " value
should be true.",
+ conf.get(YarnConfiguration.RM_ZK_CLIENT_SSL_ENABLED),
Boolean.TRUE.toString());
+ }
Review Comment:
Here, you are only testing the behavior of the YarnConfiguration class as
you set something to it, and you assert what's coming back with `conf.get()`.
In my view, this kind of test does not belong here but to the tests of
`YarnConfiguration` but I assume the `Configuration` class has a similar or
same testcase for string values.
Besides, is this config gonna be a boolean-typed or string-typed config?
There's also a method called `Configuration#setBoolean`.
Can you add 2 testcases for the SSL settings?
They should test that if the ZKCuratorManager is started with SSL disabled
by default. The other test should check if the SSL setting is set to true,
ZKCuratorManager is started with SSL enabled.
I'm not sure which test class those 2 testcases would belong to, but I
assume there's something where the Curator classes are mocked.
If not, probably you can get away with a MockRM based tests, even if the
curator classes are real instances and not mocks.
Between the two, I would prefer the former approach.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/conf/YarnConfiguration.java:
##########
@@ -853,6 +853,11 @@ public static boolean isAclEnabled(Configuration conf) {
/** Zookeeper interaction configs */
public static final String RM_ZK_PREFIX = RM_PREFIX + "zk-";
+ /** Enable Zookeeper SSL/TLS communication */
+ public static final String RM_ZK_CLIENT_SSL_ENABLED =
+ RM_ZK_PREFIX + "client-ssl.enabled";
Review Comment:
Nit: Can fit into a single line.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
