[GitHub] [nifi] jfrazee commented on a change in pull request #4613: NiFi-7819 - Add Zookeeper client TLS (external zookeeper) for cluster state management

[GitBox <gi...@apache.org>]

jfrazee commented on a change in pull request #4613:
URL: https://github.com/apache/nifi/pull/4613#discussion_r516834908



##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/state/providers/zookeeper/ZooKeeperStateProvider.java
##########
@@ -133,20 +146,54 @@ public ZooKeeperStateProvider() {
         return properties;
     }
 
-
     @Override
     public synchronized void init(final StateProviderInitializationContext context) {
         connectionString = context.getProperty(CONNECTION_STRING).getValue();
         rootNode = context.getProperty(ROOT_NODE).getValue();
         timeoutMillis = context.getProperty(SESSION_TIMEOUT).asTimePeriod(TimeUnit.MILLISECONDS).intValue();
 
+        final Properties stateProviderProperties = new Properties();
+        stateProviderProperties.setProperty(NiFiProperties.ZOOKEEPER_SESSION_TIMEOUT, String.valueOf(timeoutMillis));
+        stateProviderProperties.setProperty(NiFiProperties.ZOOKEEPER_CONNECT_TIMEOUT, String.valueOf(timeoutMillis));
+        stateProviderProperties.setProperty(NiFiProperties.ZOOKEEPER_ROOT_NODE, rootNode);
+        stateProviderProperties.setProperty(NiFiProperties.ZOOKEEPER_CONNECT_STRING, connectionString);
+
+        zooKeeperClientConfig = ZooKeeperClientConfig.createConfig(combineProperties(nifiProperties, stateProviderProperties));
         if (context.getProperty(ACCESS_CONTROL).getValue().equalsIgnoreCase(CREATOR_ONLY.getValue())) {
             acl = Ids.CREATOR_ALL_ACL;
         } else {
             acl = Ids.OPEN_ACL_UNSAFE;
         }
     }
 
+    /**
+     * Combine properties from NiFiProperties and additional properties, allowing these additional properties to override settings
+     * in the given NiFiProperties.
+     * @param nifiProps A NiFiProperties to be combined with some additional properties
+     * @param additionalProperties Additional properties that can be used to override properties in the given NiFiProperties
+     * @return NiFiProperties containing the combined properties
+     */
+    static NiFiProperties combineProperties(NiFiProperties nifiProps, Properties additionalProperties) {
+        return new NiFiProperties() {
+            @Override
+            public String getProperty(String key) {
+                String property = additionalProperties.getProperty(key);
+                if(nifiProps != null) {
+                    return property != null ? property : nifiProps.getProperty(key);
+                } else {
+                    return null;
+                }
+            }

Review comment:
       If I'm reading this right, when nifiProps is null then this returns null no matter whether there's an additional property set or not. Is this the desired behavior? As an override I would have expected the additional property to be returned and this could simplify to:
   ```
   return additionalProperties.getProperty(key, nifiProps != null ? nifiProps.getProperty(key) : null);
   ```




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org