bbende commented on a change in pull request #4613:
URL: https://github.com/apache/nifi/pull/4613#discussion_r519984662
##########
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 +147,52 @@ 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));
Review comment:
I know the main intent here was to piggy back off the TLS properties so
that they didn't need to be redefined in `state-management.xml`, but what about
the the existing properties like the connection string, root node, and session
timeout? should you be able to leave all of these blank in
`state-manegement.xml `and have it use the same values from `nifi.properties`?
As a test, I left the connection string blank in `state-management.xml` (not
sure why the descriptor was not marked required from the beginning), and it
produced this error:
```
Caused by: java.lang.IllegalStateException: The
'nifi.zookeeper.connect.string' property is not set in nifi.properties
at
org.apache.nifi.controller.cluster.ZooKeeperClientConfig.createConfig(ZooKeeperClientConfig.java:154)
at
org.apache.nifi.controller.state.providers.zookeeper.ZooKeeperStateProvider.init(ZooKeeperStateProvider.java:162)
at
org.apache.nifi.controller.state.providers.AbstractStateProvider.initialize(AbstractStateProvider.java:34)
at
org.apache.nifi.controller.state.manager.StandardStateManagerProvider$1.initialize(StandardStateManagerProvider.java:339)
at
org.apache.nifi.controller.state.manager.StandardStateManagerProvider.createStateProvider(StandardStateManagerProvider.java:239)
at
org.apache.nifi.controller.state.manager.StandardStateManagerProvider.createClusteredStateProvider(StandardStateManagerProvider.java:118)
at
org.apache.nifi.controller.state.manager.StandardStateManagerProvider.create(StandardStateManagerProvider.java:96)
at
org.apache.nifi.cluster.coordination.node.NodeClusterCoordinator.<init>(NodeClusterCoordinator.java:130)
at
org.apache.nifi.cluster.spring.NodeClusterCoordinatorFactoryBean.getObject(NodeClusterCoordinatorFactoryBean.java:53)
at
org.apache.nifi.cluster.spring.NodeClusterCoordinatorFactoryBean.getObject(NodeClusterCoordinatorFactoryBean.java:35)
at
org.springframework.beans.factory.support.FactoryBeanRegistrySupport.doGetObjectFromFactoryBean(FactoryBeanRegistrySupport.java:178)
... 60 common frames omitted
```
What ends up happening is that the `stateProviderProperties` map has empty
string for connect string, so it doesn't fallback to `nifi.properties` since
its not null.
I'm fine with it being required in state-management, but we probably would
want to validate that before passing into `ZooKeeperClientConfig`, otherwise
the error message is a little misleading since I do have
`nifi.zookeeper.connect.string` set in `nifi.properties`.
----------------------------------------------------------------
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:
[email protected]
