[GitHub] [nifi] markap14 commented on a diff in pull request #6115: NIFI-10108 Processor scheduling via parameter
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
computeValidationErrors(final ValidationC
}
@Override
-protected List validateConfig() {
+public List validateConfig() {
final List 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 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 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.
+ *
[GitHub] [nifi] markap14 commented on a diff in pull request #6115: NIFI-10108 Processor scheduling via parameter
markap14 commented on code in PR #6115:
URL: https://github.com/apache/nifi/pull/6115#discussion_r894840416
##
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/StandardProcessorNode.java:
##
@@ -1197,6 +1208,34 @@ protected Collection
computeValidationErrors(final ValidationC
return results;
}
+@Override
+protected List validateConfig() {
+
+final List results = new ArrayList<>();
+final ParameterContext parameterContext = getParameterContext();
+
+if (parameterContext == null && !this.parameterReferences.isEmpty()) {
+results.add(new ValidationResult.Builder()
+.subject(getName())
+.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() ) {
Review Comment:
It should also be invalid if it references a sensitive parameter.
##
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/StandardProcessorNode.java:
##
@@ -1197,6 +1208,34 @@ protected Collection
computeValidationErrors(final ValidationC
return results;
}
+@Override
+protected List validateConfig() {
+
+final List results = new ArrayList<>();
+final ParameterContext parameterContext = getParameterContext();
+
+if (parameterContext == null && !this.parameterReferences.isEmpty()) {
+results.add(new ValidationResult.Builder()
+.subject(getName())
Review Comment:
Subject here should be what is validated. So "Run Schedule" or similar.
Should also include `.input(runSchedule)` or something analogous to convey
what the value is (but not expose the evaluated value)
##
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/service/StandardControllerServiceNode.java:
##
@@ -504,6 +505,11 @@ public ValidationState performValidation(final
ValidationContext validationConte
return state;
}
+@Override
+protected List validateConfig() {
+return Collections.EMPTY_LIST;
Review Comment:
`Collections.EMPTY_LIST` is discouraged. We should instead use
`Collections.emptyList()` as it adheres properly to generics
##
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/StandardProcessorNode.java:
##
@@ -505,24 +510,30 @@ public String getSchedulingPeriod() {
@Override
@SuppressWarnings("deprecation")
-public synchronized void setScheduldingPeriod(final String
schedulingPeriod) {
+public synchronized void setSchedulingPeriod(final String
schedulingPeriod) {
if (isRunning()) {
throw new IllegalStateException("Cannot modify Processor
configuration while the Processor is running");
}
+final String evaluatedSchedulingPeriod =
evaluateSchedulingPeriodFromParameter(schedulingPeriod);
+final ParameterParser parameterParser = new
ExpressionLanguageAgnosticParameterParser();
+final ParameterTokenList parameterTokenList =
parameterParser.parseTokens(schedulingPeriod);
+
+parameterReferences = new
ArrayList<>(parameterTokenList.toReferenceList());
+
switch (schedulingStrategy) {
case CRON_DRIVEN: {
try {
-new CronExpression(schedulingPeriod);
+new CronExpression(evaluatedSchedulingPeriod);
} catch (final Exception e) {
throw new IllegalArgumentException(
-"Scheduling Period is not a valid cron expression: " +
schedulingPeriod);
+"Scheduling Period is not a valid cron expression: " +
evaluatedSchedulingPeriod);
}
}
break;
case PRIMARY_NODE_ONLY:
case TIMER_DRIVEN: {
-final long schedulingNanos =
FormatUtils.getTimeDuration(requireNonNull(schedulingPeriod),
+final long schedulingNanos =
FormatUtils.getTimeDuration(requireNonNull(evaluatedSchedulingPeriod),
TimeUnit.NANOSECONDS);
if (schedulingNanos < 0) {
throw new IllegalArgumentException("Scheduling Period must be
positive");
Review Comment:
Because these values can change outside of this method now, by changing a
parameter, we need to perform these checks during validation. Because we'll be
checking it there, and because we shouldn't throw an Exception if referencing
