exceptionfactory commented on code in PR #8240:
URL: https://github.com/apache/nifi/pull/8240#discussion_r1450663163
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-kubernetes-bundle/nifi-framework-kubernetes-state-provider/src/main/java/org/apache/nifi/kubernetes/state/provider/KubernetesConfigMapStateProvider.java:
##########
@@ -40,26 +41,35 @@
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import org.apache.nifi.components.AbstractConfigurableComponent;
+import org.apache.nifi.components.PropertyDescriptor;
import org.apache.nifi.components.state.Scope;
import org.apache.nifi.components.state.StateMap;
import org.apache.nifi.components.state.StateProvider;
import org.apache.nifi.components.state.StateProviderInitializationContext;
import org.apache.nifi.kubernetes.client.ServiceAccountNamespaceProvider;
import org.apache.nifi.kubernetes.client.StandardKubernetesClientProvider;
import org.apache.nifi.logging.ComponentLog;
+import org.apache.nifi.processor.util.StandardValidators;
/**
* State Provider implementation based on Kubernetes ConfigMaps with Base64
encoded keys to meet Kubernetes constraints
*/
public class KubernetesConfigMapStateProvider extends
AbstractConfigurableComponent implements StateProvider {
+ static final PropertyDescriptor RESOURCE_PREFIX = new
PropertyDescriptor.Builder()
+ .name("Resource Prefix")
+ .description("The resource prefix to add to the beginning of the
kubernetes resource names, followed by a hyphen")
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+ .required(false)
+ .build();
+
private static final int MAX_UPDATE_ATTEMPTS = 5;
private static final Scope[] SUPPORTED_SCOPES = { Scope.CLUSTER };
private static final Charset KEY_CHARACTER_SET = StandardCharsets.UTF_8;
- private static final String CONFIG_MAP_NAME_FORMAT = "nifi-component-%s";
+ private static String CONFIG_MAP_NAME_FORMAT;
- private static final Pattern CONFIG_MAP_NAME_PATTERN =
Pattern.compile("^nifi-component-(.+)$");
+ private static Pattern CONFIG_MAP_NAME_PATTERN;
Review Comment:
These variables should not be `static` and should instead be changed to
instance variables since they are different per instance of the Provider.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-kubernetes-bundle/nifi-framework-kubernetes-state-provider/src/main/java/org/apache/nifi/kubernetes/state/provider/KubernetesConfigMapStateProvider.java:
##########
@@ -40,26 +41,35 @@
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import org.apache.nifi.components.AbstractConfigurableComponent;
+import org.apache.nifi.components.PropertyDescriptor;
import org.apache.nifi.components.state.Scope;
import org.apache.nifi.components.state.StateMap;
import org.apache.nifi.components.state.StateProvider;
import org.apache.nifi.components.state.StateProviderInitializationContext;
import org.apache.nifi.kubernetes.client.ServiceAccountNamespaceProvider;
import org.apache.nifi.kubernetes.client.StandardKubernetesClientProvider;
import org.apache.nifi.logging.ComponentLog;
+import org.apache.nifi.processor.util.StandardValidators;
/**
* State Provider implementation based on Kubernetes ConfigMaps with Base64
encoded keys to meet Kubernetes constraints
*/
public class KubernetesConfigMapStateProvider extends
AbstractConfigurableComponent implements StateProvider {
+ static final PropertyDescriptor RESOURCE_PREFIX = new
PropertyDescriptor.Builder()
Review Comment:
Recommend naming this more specifically:
```suggestion
static final PropertyDescriptor CONFIG_MAP_NAME_PREFIX = new
PropertyDescriptor.Builder()
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-kubernetes-bundle/nifi-framework-kubernetes-state-provider/src/main/java/org/apache/nifi/kubernetes/state/provider/KubernetesConfigMapStateProvider.java:
##########
@@ -40,26 +41,35 @@
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import org.apache.nifi.components.AbstractConfigurableComponent;
+import org.apache.nifi.components.PropertyDescriptor;
import org.apache.nifi.components.state.Scope;
import org.apache.nifi.components.state.StateMap;
import org.apache.nifi.components.state.StateProvider;
import org.apache.nifi.components.state.StateProviderInitializationContext;
import org.apache.nifi.kubernetes.client.ServiceAccountNamespaceProvider;
import org.apache.nifi.kubernetes.client.StandardKubernetesClientProvider;
import org.apache.nifi.logging.ComponentLog;
+import org.apache.nifi.processor.util.StandardValidators;
/**
* State Provider implementation based on Kubernetes ConfigMaps with Base64
encoded keys to meet Kubernetes constraints
*/
public class KubernetesConfigMapStateProvider extends
AbstractConfigurableComponent implements StateProvider {
+ static final PropertyDescriptor RESOURCE_PREFIX = new
PropertyDescriptor.Builder()
+ .name("Resource Prefix")
Review Comment:
```suggestion
.name("ConfigMap Name Prefix")
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-kubernetes-bundle/nifi-framework-kubernetes-leader-election/src/main/java/org/apache/nifi/kubernetes/leader/election/KubernetesLeaderElectionManager.java:
##########
@@ -73,10 +74,13 @@ public class KubernetesLeaderElectionManager extends
TrackedLeaderElectionManage
private final LeaderElectionCommandProvider leaderElectionCommandProvider;
+ private final String prefix;
Review Comment:
Recommend naming this `roleIdPrefix` for clarity.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-kubernetes-bundle/nifi-framework-kubernetes-state-provider/src/main/java/org/apache/nifi/kubernetes/state/provider/KubernetesConfigMapStateProvider.java:
##########
@@ -99,6 +116,11 @@ public void initialize(final
StateProviderInitializationContext context) {
this.logger = context.getLogger();
this.kubernetesClient = getKubernetesClient();
this.namespace = new ServiceAccountNamespaceProvider().getNamespace();
+
+ String resourcePrefix = context.getProperty(RESOURCE_PREFIX).isSet() ?
context.getProperty(RESOURCE_PREFIX).getValue() : null;
+ CONFIG_MAP_NAME_FORMAT = resourcePrefix != null ?
String.format("%s-nifi-component-%%s", resourcePrefix) : "nifi-component-%s";
+ CONFIG_MAP_NAME_PATTERN = Pattern.compile(resourcePrefix != null ?
+ String.format("^%s-nifi-component-(.+)$", resourcePrefix) :
"^nifi-component-(.+)$");
Review Comment:
Recommend pulling out some of these string values, particularly
`nifi-component` to static values that can be used to build the final pattern
and format properties.
##########
nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/NiFiProperties.java:
##########
@@ -326,6 +326,9 @@ public class NiFiProperties extends ApplicationProperties {
public static final String PYTHON_CONTROLLER_DEBUGPY_HOST =
"nifi.python.controller.debugpy.host";
public static final String PYTHON_CONTROLLER_DEBUGPY_LOGS_DIR =
"nifi.python.controller.debugpy.logs.directory";
+ // kubernetes properties
+ public static final String KUBERNETES_RESOURCE_PREFIX =
"nifi.kubernetes.resource.prefix";
Review Comment:
Recommend naming this more specifically for clarity:
```suggestion
public static final String
CLUSTER_LEADER_ELECTION_KUBERNETES_LEASE_PREFIX =
"nifi.cluster.leader.election.kubernetes.lease.prefix";
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-nar-utils/src/test/resources/nifi.properties:
##########
@@ -181,4 +181,7 @@ nifi.kerberos.service.keytab.location=
# kerberos spnego principal #
nifi.kerberos.spnego.principal=
nifi.kerberos.spnego.keytab.location=
-nifi.kerberos.spnego.authentication.expiration=12 hours
\ No newline at end of file
+nifi.kerberos.spnego.authentication.expiration=12 hours
+
+# kubernetes #
+nifi.kubernetes.resource.prefix=
Review Comment:
It is not necessary to add the empty placeholder value to these test
properties files.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-kubernetes-bundle/nifi-framework-kubernetes-state-provider/src/main/java/org/apache/nifi/kubernetes/state/provider/KubernetesConfigMapStateProvider.java:
##########
@@ -40,26 +41,35 @@
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import org.apache.nifi.components.AbstractConfigurableComponent;
+import org.apache.nifi.components.PropertyDescriptor;
import org.apache.nifi.components.state.Scope;
import org.apache.nifi.components.state.StateMap;
import org.apache.nifi.components.state.StateProvider;
import org.apache.nifi.components.state.StateProviderInitializationContext;
import org.apache.nifi.kubernetes.client.ServiceAccountNamespaceProvider;
import org.apache.nifi.kubernetes.client.StandardKubernetesClientProvider;
import org.apache.nifi.logging.ComponentLog;
+import org.apache.nifi.processor.util.StandardValidators;
/**
* State Provider implementation based on Kubernetes ConfigMaps with Base64
encoded keys to meet Kubernetes constraints
*/
public class KubernetesConfigMapStateProvider extends
AbstractConfigurableComponent implements StateProvider {
+ static final PropertyDescriptor RESOURCE_PREFIX = new
PropertyDescriptor.Builder()
+ .name("Resource Prefix")
+ .description("The resource prefix to add to the beginning of the
kubernetes resource names, followed by a hyphen")
Review Comment:
This should be clarified to indicate that it applies to ConfigMap entries:
```suggestion
.description("Optional prefix that the Provider will prepend to
Kubernetes ConfigMap names. The resulting ConfigMap name will contain
nifi-component and the component identifier.")
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
