exceptionfactory commented on code in PR #7874:
URL: https://github.com/apache/nifi/pull/7874#discussion_r1366119104
##########
nifi-api/src/main/java/org/apache/nifi/migration/PropertyConfiguration.java:
##########
@@ -128,10 +128,83 @@ default Optional<String>
getPropertyValue(PropertyDescriptor descriptor) {
return getPropertyValue(descriptor.getName());
}
+ /**
+ * Returns an optional value representing the "raw" value of the property
with the given name. The "raw" value is
+ * the value before any parameters are substituted.
+ *
+ * @param propertyName the name of the property
+ * @return an empty optional if the value is null or unset, else an
Optional representing the configured value
+ */
+ Optional<String> getRawPropertyValue(String propertyName);
+
+ /**
+ * Returns an optional value representing the "raw" value of the property
identified by the given descriptor. The "raw" value is
+ * the value before any parameters are substituted.
+ *
+ * @param descriptor the descriptor that identifies the property
+ * @return an empty optional if the value is null or unset, else an
Optional representing the configured value
+ */
+ default Optional<String> getRawPropertyValue(PropertyDescriptor
descriptor) {
+ return getRawPropertyValue(descriptor.getName());
+ }
+
/**
* Returns a map containing all of the configured properties
* @return a Map containing the names and values of all configured
properties
*/
Map<String, String> getProperties();
+ /**
+ * Returns a map containing all of the raw property values
+ *
+ * @return a Map containing the names and values of all configured
properties
+ */
+ Map<String, String> getRawProperties();
+
+ /**
+ * <p>
+ * Creates a new Controller Service of the given type and configures it
with the given property values. Note that if a Controller Service
+ * already exists within the same scope and with the same implementation
and configuration, a new service may not be created and instead
+ * the existing service may be used.
+ * </p>
+ *
+ * <p>
+ * This allows for properties that were previously defined in the
extension to be moved to a Controller Service. For example,
+ * consider a Processor that has "Username" and "Password" properties. In
the next version of the Processor, we want to support
+ * multiple types of authentication, and we delegate the authentication to
a Controller Service. Consider that the Controller Service
+ * implementation we wish to use has a classname of {@code
org.apache.nifi.services.authentication.UsernamePassword}. We might then
+ * use this method as such:
+ * </p>
+ *
+ * <pre><code>
+ * // Create a new Controller Service ofo type
org.apache.nifi.services.authentication.UsernamePassword whose Username and
Password
Review Comment:
```suggestion
* // Create a new Controller Service of type
org.apache.nifi.services.authentication.UsernamePassword whose Username and
Password
```
##########
nifi-mock/src/main/java/org/apache/nifi/util/MockPropertyConfiguration.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.nifi.util;
+
+import org.apache.nifi.migration.PropertyConfiguration;
+
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.UUID;
+
+public class MockPropertyConfiguration implements PropertyConfiguration {
+ private final Map<String, String> propertyRenames = new HashMap<>();
+ private final Set<String> propertiesRemoved = new HashSet<>();
+ private final Set<String> propertiesUpdated = new HashSet<>();
+ private final Map<String, String> rawProperties;
+ private final Set<CreatedControllerService> createdControllerServices =
new HashSet<>();
Review Comment:
Recommend using `Linked` implementations to avoid unexpected ordering
differences in some potential comparisons.
```suggestion
private final Map<String, String> propertyRenames = new
LinkedHashMap<>();
private final Set<String> propertiesRemoved = new LinkedHashSet<>();
private final Set<String> propertiesUpdated = new LinkedHashSet<>();
private final Map<String, String> rawProperties;
private final Set<CreatedControllerService> createdControllerServices =
new LinkedHashSet<>();
```
##########
nifi-mock/src/main/java/org/apache/nifi/util/MockPropertyConfiguration.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.nifi.util;
+
+import org.apache.nifi.migration.PropertyConfiguration;
+
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.UUID;
+
+public class MockPropertyConfiguration implements PropertyConfiguration {
+ private final Map<String, String> propertyRenames = new HashMap<>();
+ private final Set<String> propertiesRemoved = new HashSet<>();
+ private final Set<String> propertiesUpdated = new HashSet<>();
+ private final Map<String, String> rawProperties;
+ private final Set<CreatedControllerService> createdControllerServices =
new HashSet<>();
+
+
+ public MockPropertyConfiguration(final Map<String, String> propertyValues)
{
+ this.rawProperties = new HashMap<>(propertyValues);
Review Comment:
```suggestion
this.rawProperties = new LinkedHashMap<>(propertyValues);
```
##########
nifi-mock/src/main/java/org/apache/nifi/util/TestRunner.java:
##########
@@ -1064,4 +1064,6 @@ void assertAttributes(
* @param eventType Provenance event type
*/
void assertProvenanceEvent(ProvenanceEventType eventType);
+
Review Comment:
It would be helpful to add a basic Java Doc comment on this method to assist
developers in using this method.
##########
nifi-api/src/main/java/org/apache/nifi/migration/PropertyConfiguration.java:
##########
@@ -128,10 +128,83 @@ default Optional<String>
getPropertyValue(PropertyDescriptor descriptor) {
return getPropertyValue(descriptor.getName());
}
+ /**
+ * Returns an optional value representing the "raw" value of the property
with the given name. The "raw" value is
+ * the value before any parameters are substituted.
+ *
+ * @param propertyName the name of the property
+ * @return an empty optional if the value is null or unset, else an
Optional representing the configured value
+ */
+ Optional<String> getRawPropertyValue(String propertyName);
+
+ /**
+ * Returns an optional value representing the "raw" value of the property
identified by the given descriptor. The "raw" value is
+ * the value before any parameters are substituted.
+ *
+ * @param descriptor the descriptor that identifies the property
+ * @return an empty optional if the value is null or unset, else an
Optional representing the configured value
+ */
+ default Optional<String> getRawPropertyValue(PropertyDescriptor
descriptor) {
+ return getRawPropertyValue(descriptor.getName());
+ }
+
/**
* Returns a map containing all of the configured properties
* @return a Map containing the names and values of all configured
properties
*/
Map<String, String> getProperties();
+ /**
+ * Returns a map containing all of the raw property values
+ *
+ * @return a Map containing the names and values of all configured
properties
+ */
+ Map<String, String> getRawProperties();
+
+ /**
+ * <p>
+ * Creates a new Controller Service of the given type and configures it
with the given property values. Note that if a Controller Service
+ * already exists within the same scope and with the same implementation
and configuration, a new service may not be created and instead
+ * the existing service may be used.
+ * </p>
+ *
+ * <p>
+ * This allows for properties that were previously defined in the
extension to be moved to a Controller Service. For example,
+ * consider a Processor that has "Username" and "Password" properties. In
the next version of the Processor, we want to support
+ * multiple types of authentication, and we delegate the authentication to
a Controller Service. Consider that the Controller Service
+ * implementation we wish to use has a classname of {@code
org.apache.nifi.services.authentication.UsernamePassword}. We might then
+ * use this method as such:
+ * </p>
+ *
+ * <pre><code>
+ * // Create a new Controller Service ofo type
org.apache.nifi.services.authentication.UsernamePassword whose Username and
Password
+ * // properties match those currently configured for this Processor.
+ * final Map<String, String> serviceProperties =
Map.of("Username", propertyConfiguration.getRawPropertyValue("Username"),
+ * "Password",
propertyConfiguration.getRawPropertyValue("Password"));
+ * final String serviceId =
propertyConfiguration.createControllerService("org.apache.nifi.services.authentication.UsernamePassword",
serviceProperties);
+ *
+ * // Set our Authentication Service property to point to this new
service.
+ * propertyConfiguration.setProperty(AUTHENTICATION_SERVICE,
serviceId);
+ *
+ * // Remove the Username and Password properties from this Processor,
since we are now going to use then Authentication Service.
+ * propertyConfiguration.removeProperty("Username");
+ * propertyConfiguration.removeProperty("Password");
+ * </code></pre>
+ *
+ * <p>
+ * Note the use of {@link #getRawPropertyValue(String)} here instead of
{@link #getPropertyValue(String)}. Because we want to set
+ * the new Controller Service's value to the same value as is currently
configured for the Processor's "Username" and "Password" properties,
+ * we use {@link #getRawPropertyValue(String)}. This ensures that if the
Processor is configured using Parameters, those Parameter
+ * references are still held by the Controller Service.
+ * </p>
+ *
+ * <p>
+ * Also note that this method expects the classname of the implementation,
not the classname of the interface.
+ * </p>
+ *
+ * @param implementationClassName the fully qualified classname of the
Controller Service implementation
+ * @param serviceProperties the property values to configure the
newly created Controller Service with
+ * @return an identifier for the Controller Service
+ */
+ String createControllerService(String implementationClassName, Map<String,
String> serviceProperties);
Review Comment:
The implementation can throw an `IllegalArgumentException`, but that makes
sense in the end as this is an operation that must succeed based on requested
implementation.
This ends up resulting a stronger requirement between a particular component
and a Controller Service implementation, but as shown in the AWS use cases,
that seems reasonable.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/migration/ControllerServiceFactory.java:
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.nifi.migration;
+
+import org.apache.nifi.controller.service.ControllerServiceNode;
+
+import java.util.Map;
+
+public interface ControllerServiceFactory {
+ ControllerServiceCreationDetails creationDetails(String
implementationClassName, Map<String, String> propertyValues);
Review Comment:
Minor naming question, is there a reason for naming this `creationDetails`
as opposed to `getCreationDetails()`?
##########
nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/test/java/org/apache/nifi/processors/aws/kinesis/stream/ITPutKinesisStreamWithEndpointOverride.java:
##########
@@ -29,24 +31,24 @@
// This integration test can be run against a mock Kenesis such as
// https://github.com/mhart/kinesalite or
https://github.com/localstack/localstack
+@Disabled("Required external service be running. Needs to be updated to make
use of Localstack TestContainer")
Review Comment:
I recommend deleting this test class instead of marking it disabled. There
is not a lot included, and it would be better to start with a new integration
test class.
##########
nifi-system-tests/nifi-system-test-suite/src/test/resources/conf/default/bootstrap.conf:
##########
@@ -27,7 +27,7 @@ java.arg.3=-Xmx512m
java.arg.14=-Djava.awt.headless=true
-#java.arg.debug=-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=8002
+java.arg.debug=-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=8002
Review Comment:
Reset to commented?
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
