markap14 commented on code in PR #6115:
URL: https://github.com/apache/nifi/pull/6115#discussion_r902915510
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/StandardProcessorNode.java:
##########
@@ -1209,33 +1194,112 @@ protected Collection<ValidationResult>
computeValidationErrors(final ValidationC
}
@Override
- protected List<ValidationResult> validateConfig() {
+ public List<ValidationResult> validateConfig() {
final List<ValidationResult> results = new ArrayList<>();
final ParameterContext parameterContext = getParameterContext();
- if (parameterContext == null && !this.parameterReferences.isEmpty()) {
+ if (parameterContext == null &&
!this.configurationParameterReferences.isEmpty()) {
results.add(new ValidationResult.Builder()
- .subject(getName())
+ .subject(RUN_SCHEDULE)
+ .input("Parameter Context")
.valid(false)
.explanation("Processor configuration references one or
more Parameters but no Parameter Context is currently set on the Process
Group.")
.build());
} else {
- for (final ParameterReference paramRef : parameterReferences) {
- if
(!parameterContext.getParameter(paramRef.getParameterName()).isPresent() ) {
+ for (final ParameterReference paramRef :
configurationParameterReferences) {
+ final Optional<Parameter> parameterRef =
parameterContext.getParameter(paramRef.getParameterName());
+ if (!parameterRef.isPresent() ) {
results.add(new ValidationResult.Builder()
- .subject(getName())
+ .subject(RUN_SCHEDULE)
+ .input(paramRef.getParameterName())
.valid(false)
.explanation("Processor configuration references
Parameter '" + paramRef.getParameterName() +
"' but the currently selected Parameter
Context does not have a Parameter with that name")
.build());
+ } else {
+ final ParameterDescriptor parameterDescriptor =
parameterRef.get().getDescriptor();
+ if (parameterDescriptor.isSensitive()) {
+ results.add(new ValidationResult.Builder()
+ .subject(RUN_SCHEDULE)
+ .input(parameterDescriptor.getName())
+ .valid(false)
+ .explanation("Processor configuration cannot
reference sensitive parameters")
+ .build());
+ }
}
}
- }
+ final String schedulingPeriod = getSchedulingPeriod();
+ final String evaluatedSchedulingPeriod =
evaluateParameter(schedulingPeriod);
+
+ if (evaluatedSchedulingPeriod != null) {
Review Comment:
This should never be `null`, should it?
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/StandardProcessorNode.java:
##########
@@ -1969,17 +2033,12 @@ public void setMaxBackoffPeriod(String
maxBackoffPeriod) {
}
@Override
- public String evaluateSchedulingPeriodFromParameter(final String
schedulingPeriod) {
+ public String evaluateParameter(final String parameter) {
Review Comment:
I don't think the argument here is really a parameter is it? It's some value
that may or may not contain parameters. Perhaps should call it
`evaluateParameters` and call the argument `value`?
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractComponentNode.java:
##########
@@ -1084,12 +1092,29 @@ public void onParametersModified(final Map<String,
ParameterUpdate> updatedParam
}
// If this component is affected by the Parameter change, we need to
re-validate
+ componentAffected |=
isConfigurationParameterModified(updatedParameters);
if (componentAffected) {
logger.debug("Configuration of {} changed due to an update to
Parameter Context. Resetting validation state", this);
resetValidationState();
}
}
+ protected void increaseReferenceCounts(final String parameterName) {
+ parameterReferenceCounts.merge(parameterName, 1, (a, b) -> a == -1 ?
null : a + b);
+ }
+
+ protected void decreaseReferenceCounts(final String parameterName) {
+ parameterReferenceCounts.merge(parameterName, -1, (a, b) -> a == 1 ?
null : a + b);
+ }
+
+ /**
+ * Verifies whether any referenced configuration parameter has been
updated and its effective value has changed.
+ * If yes, the component needs to be re-validated.
+ * @param updatedParameters updated parameters
+ * @return true if the component requires re-validation
Review Comment:
With a method named `isConfigurationParameterModified`, it should not return
`true` to indicate that the component requires validation. That's simply a
side-effect. It should return `true` if any of the given parameters are used in
its configuration.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractComponentNode.java:
##########
@@ -1084,12 +1092,29 @@ public void onParametersModified(final Map<String,
ParameterUpdate> updatedParam
}
// If this component is affected by the Parameter change, we need to
re-validate
+ componentAffected |=
isConfigurationParameterModified(updatedParameters);
if (componentAffected) {
logger.debug("Configuration of {} changed due to an update to
Parameter Context. Resetting validation state", this);
resetValidationState();
}
}
+ protected void increaseReferenceCounts(final String parameterName) {
+ parameterReferenceCounts.merge(parameterName, 1, (a, b) -> a == -1 ?
null : a + b);
+ }
+
+ protected void decreaseReferenceCounts(final String parameterName) {
Review Comment:
Given that this is always changing the value by `1`, it probably is best to
call these `incrementReferenceCounts` and `decrementReferenceCounts`
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/connectable/Connectable.java:
##########
@@ -311,5 +311,5 @@ default boolean isSessionBatchingSupported() {
void setMaxBackoffPeriod(String maxBackoffPeriod);
- String evaluateSchedulingPeriodFromParameter(String schedulingPeriod);
+ String evaluateParameter(String parameter);
Review Comment:
As commented, based on the way this is used, I think the signature should be
`String evaluateParameters(String value);`
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/parameter/StandardParameterReferenceManager.java:
##########
@@ -128,6 +128,14 @@ private boolean isComponentReferencing(final ComponentNode
componentNode, final
}
}
+ if (componentNode instanceof ProcessorNode) {
Review Comment:
We shouldn't be treating ProcessorNode specially. Perhaps it makes more
sense to get rid of this `isComponentReferencing` method from
`StandardParameterReferenceManager` and instead add a
`isReferencingParameter(String parameterName)` to `ComponentNode`. Then the
existing implementation would move to `AbstractComponentNode` and
`StandardProcessorNode` can then override the method, call the
`super.isReferencingParameter` and if false check its
`configurationParameterReferences`
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/StandardNiFiServiceFacade.java:
##########
@@ -1462,6 +1459,25 @@ private Set<AffectedComponentEntity>
getComponentsAffectedByParameterContextUpda
return dtoFactory.createAffectedComponentEntities(affectedComponents,
revisionManager);
}
+ private void getAffectedComponentsByParameterUpdate(final
ParameterContextDTO parameterContextDto, final Set<String>
updatedParameterNames, final Set<ComponentNode> affectedComponents,
Review Comment:
We should not have a getter that doesn't return a value. Is the intent to
populate the provided `affectedComponents` set? If so, perhaps that should be
returned instead.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractComponentNode.java:
##########
@@ -1084,12 +1092,29 @@ public void onParametersModified(final Map<String,
ParameterUpdate> updatedParam
}
// If this component is affected by the Parameter change, we need to
re-validate
+ componentAffected |=
isConfigurationParameterModified(updatedParameters);
if (componentAffected) {
logger.debug("Configuration of {} changed due to an update to
Parameter Context. Resetting validation state", this);
resetValidationState();
}
}
+ protected void increaseReferenceCounts(final String parameterName) {
+ parameterReferenceCounts.merge(parameterName, 1, (a, b) -> a == -1 ?
null : a + b);
+ }
+
+ protected void decreaseReferenceCounts(final String parameterName) {
+ parameterReferenceCounts.merge(parameterName, -1, (a, b) -> a == 1 ?
null : a + b);
+ }
+
+ /**
+ * Verifies whether any referenced configuration parameter has been
updated and its effective value has changed.
+ * If yes, the component needs to be re-validated.
+ * @param updatedParameters updated parameters
+ * @return true if the component requires re-validation
+ **/
+ public abstract boolean isConfigurationParameterModified(final Map<String,
ParameterUpdate> updatedParameters);
Review Comment:
I'm not sure exactly what a "configuration parameter" is - i presume here we
mean a parameter that is used within the configuration, other than properties,
such as the Scheduling Period. But not sure that this comes across from the
code. Also not entirely sure that this is something that we should distinguish
between - parameters that are used by properties and parameters that are used
elsewhere. Perhaps this would call for a slight refactoring elsewhere but not
sure that we should distinguish between the two. Should we just have a
`Set<String> getReferencedParameterNames()` on ComponentNode? Would that give
us everything that we need?
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/ProcessorNode.java:
##########
@@ -300,4 +301,12 @@ public abstract CompletableFuture<Void>
stop(ProcessScheduler processScheduler,
* @param context The ProcessContext associated with the Processor
configuration
*/
public abstract void onConfigurationRestored(ProcessContext context);
+
+ /**
+ * Returns those parameter references which configures the Processor
through Parameters. This is for Processor configuration only,
+ * Processor Property Parameter references stored and handled separately.
+ *
+ * @return the configuration Parameter references for this Processor
+ */
+ public abstract List<ParameterReference>
getConfigurationParameterReferences();
Review Comment:
Not sure that the naming convention here makes sense. Parameters are always
used for configuration. Processor properties are very much configuration.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/StandardNiFiServiceFacade.java:
##########
@@ -1462,6 +1459,25 @@ private Set<AffectedComponentEntity>
getComponentsAffectedByParameterContextUpda
return dtoFactory.createAffectedComponentEntities(affectedComponents,
revisionManager);
}
+ private void getAffectedComponentsByParameterUpdate(final
ParameterContextDTO parameterContextDto, final Set<String>
updatedParameterNames, final Set<ComponentNode> affectedComponents,
Review Comment:
Perhaps this should be `getComponentsAffectedBy...`
--
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]
