You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/03/11 18:18:33 UTC
[GitHub] [nifi] markap14 opened a new pull request #4134: NIFI-7242: When a Parameter is changed, any property referencing that…
markap14 opened a new pull request #4134: NIFI-7242: When a Parameter is changed, any property referencing that…
URL: https://github.com/apache/nifi/pull/4134
… parameter should have its #onPropertyModified method called. Also renamed Accumulo tests to integration tests because they start embedded servers and connect to them, which caused failures in my environment. Also fixed a bug in TestLengthDelimitedJournal because it was resulting in failures when building locally as well.
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
#### Description of PR
_Enables X functionality; fixes bug NIFI-YYYY._
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with **NIFI-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `master`)?
- [ ] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] Have you verified that the full build is successful on both JDK 8 and JDK 11?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] pvillard31 commented on issue #4134: NIFI-7242: When a Parameter is changed, any property referencing that…
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on issue #4134: NIFI-7242: When a Parameter is changed, any property referencing that…
URL: https://github.com/apache/nifi/pull/4134#issuecomment-597836252
Code change looks good to me, builds are green, I tested the case I initially reported in the JIRA and this now works as expected. Merging to master. Thanks @markap14 !
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] markap14 commented on a change in pull request #4134: NIFI-7242: When a Parameter is changed, any property referencing that…
Posted by GitBox <gi...@apache.org>.
markap14 commented on a change in pull request #4134: NIFI-7242: When a Parameter is changed, any property referencing that…
URL: https://github.com/apache/nifi/pull/4134#discussion_r391181945
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractComponentNode.java
##########
@@ -815,12 +818,110 @@ public PropertyDescriptor getPropertyDescriptor(final String name) {
}
- private void onPropertyModified(final PropertyDescriptor descriptor, final String oldValue, final String newValue) {
+ protected void onPropertyModified(final PropertyDescriptor descriptor, final String oldValue, final String newValue) {
try (final NarCloseable narCloseable = NarCloseable.withComponentNarLoader(extensionManager, getComponent().getClass(), getComponent().getIdentifier())) {
getComponent().onPropertyModified(descriptor, oldValue, newValue);
}
}
+ @Override
+ public void onParametersModified(final Map<String, ParameterUpdate> updatedParameters) {
+ // If the component doesn't reference any parameters, then there's nothing to be done.
+ if (!isReferencingParameter()) {
+ return;
+ }
+
+ final ParameterLookup previousParameterLookup = createParameterLookupForPreviousValues(updatedParameters);
+
+ // For any Property that references an updated Parameter, we need to call onPropertyModified().
+ // Additionally, we need to trigger validation to run if this component is affected by the parameter update.
+ boolean componentAffected = false;
+ for (final Map.Entry<PropertyDescriptor, PropertyConfiguration> entry : properties.entrySet()) {
+ final PropertyDescriptor propertyDescriptor = entry.getKey();
+ final PropertyConfiguration configuration = entry.getValue();
+
+ // Determine if this property is affected by the Parameter Update
+ boolean propertyAffected = false;
+ final List<ParameterReference> parameterReferences = configuration.getParameterReferences();
+ for (final ParameterReference reference : parameterReferences) {
+ final String referencedParamName = reference.getParameterName();
+ if (updatedParameters.containsKey(referencedParamName)) {
+ propertyAffected = true;
+ componentAffected = true;
+ break;
+ }
+ }
+
+ if (propertyAffected) {
+ final String previousValue = configuration.getEffectiveValue(previousParameterLookup);
+ final String updatedValue = configuration.getEffectiveValue(getParameterContext());
+
+ // Check if the value of the property is truly affected. It's possible that we could have a property configured as something like "#{a}#{b}"
+ // Where parameter a = "abc-" and b = "cba". The update could change a to "abc" and b to "-cba". As a result, the property value previously was "abc-cba" and still is.
+ // In such a case, we should not call onPropertyModified.
+ final boolean propertyUpdated = !Objects.equals(previousValue, updatedValue);
+ if (propertyUpdated) {
+ try {
+ logger.debug("Parameter Context updated, resulting in property {} of {} changing. Calling onPropertyModified().", propertyDescriptor, this);
Review comment:
This is fine to log if it's sensitive. We're not logging the value, just the descriptor.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] asfgit closed pull request #4134: NIFI-7242: When a Parameter is changed, any property referencing that…
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4134: NIFI-7242: When a Parameter is changed, any property referencing that…
URL: https://github.com/apache/nifi/pull/4134
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] pvillard31 commented on a change in pull request #4134: NIFI-7242: When a Parameter is changed, any property referencing that…
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on a change in pull request #4134: NIFI-7242: When a Parameter is changed, any property referencing that…
URL: https://github.com/apache/nifi/pull/4134#discussion_r391179441
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractComponentNode.java
##########
@@ -815,12 +818,110 @@ public PropertyDescriptor getPropertyDescriptor(final String name) {
}
- private void onPropertyModified(final PropertyDescriptor descriptor, final String oldValue, final String newValue) {
+ protected void onPropertyModified(final PropertyDescriptor descriptor, final String oldValue, final String newValue) {
try (final NarCloseable narCloseable = NarCloseable.withComponentNarLoader(extensionManager, getComponent().getClass(), getComponent().getIdentifier())) {
getComponent().onPropertyModified(descriptor, oldValue, newValue);
}
}
+ @Override
+ public void onParametersModified(final Map<String, ParameterUpdate> updatedParameters) {
+ // If the component doesn't reference any parameters, then there's nothing to be done.
+ if (!isReferencingParameter()) {
+ return;
+ }
+
+ final ParameterLookup previousParameterLookup = createParameterLookupForPreviousValues(updatedParameters);
+
+ // For any Property that references an updated Parameter, we need to call onPropertyModified().
+ // Additionally, we need to trigger validation to run if this component is affected by the parameter update.
+ boolean componentAffected = false;
+ for (final Map.Entry<PropertyDescriptor, PropertyConfiguration> entry : properties.entrySet()) {
+ final PropertyDescriptor propertyDescriptor = entry.getKey();
+ final PropertyConfiguration configuration = entry.getValue();
+
+ // Determine if this property is affected by the Parameter Update
+ boolean propertyAffected = false;
+ final List<ParameterReference> parameterReferences = configuration.getParameterReferences();
+ for (final ParameterReference reference : parameterReferences) {
+ final String referencedParamName = reference.getParameterName();
+ if (updatedParameters.containsKey(referencedParamName)) {
+ propertyAffected = true;
+ componentAffected = true;
+ break;
+ }
+ }
+
+ if (propertyAffected) {
+ final String previousValue = configuration.getEffectiveValue(previousParameterLookup);
+ final String updatedValue = configuration.getEffectiveValue(getParameterContext());
+
+ // Check if the value of the property is truly affected. It's possible that we could have a property configured as something like "#{a}#{b}"
+ // Where parameter a = "abc-" and b = "cba". The update could change a to "abc" and b to "-cba". As a result, the property value previously was "abc-cba" and still is.
+ // In such a case, we should not call onPropertyModified.
+ final boolean propertyUpdated = !Objects.equals(previousValue, updatedValue);
+ if (propertyUpdated) {
+ try {
+ logger.debug("Parameter Context updated, resulting in property {} of {} changing. Calling onPropertyModified().", propertyDescriptor, this);
Review comment:
I believe we don't want that in case this is a sensitive parameter, right?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] pvillard31 commented on a change in pull request #4134: NIFI-7242: When a Parameter is changed, any property referencing that…
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on a change in pull request #4134: NIFI-7242: When a Parameter is changed, any property referencing that…
URL: https://github.com/apache/nifi/pull/4134#discussion_r391184601
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractComponentNode.java
##########
@@ -815,12 +818,110 @@ public PropertyDescriptor getPropertyDescriptor(final String name) {
}
- private void onPropertyModified(final PropertyDescriptor descriptor, final String oldValue, final String newValue) {
+ protected void onPropertyModified(final PropertyDescriptor descriptor, final String oldValue, final String newValue) {
try (final NarCloseable narCloseable = NarCloseable.withComponentNarLoader(extensionManager, getComponent().getClass(), getComponent().getIdentifier())) {
getComponent().onPropertyModified(descriptor, oldValue, newValue);
}
}
+ @Override
+ public void onParametersModified(final Map<String, ParameterUpdate> updatedParameters) {
+ // If the component doesn't reference any parameters, then there's nothing to be done.
+ if (!isReferencingParameter()) {
+ return;
+ }
+
+ final ParameterLookup previousParameterLookup = createParameterLookupForPreviousValues(updatedParameters);
+
+ // For any Property that references an updated Parameter, we need to call onPropertyModified().
+ // Additionally, we need to trigger validation to run if this component is affected by the parameter update.
+ boolean componentAffected = false;
+ for (final Map.Entry<PropertyDescriptor, PropertyConfiguration> entry : properties.entrySet()) {
+ final PropertyDescriptor propertyDescriptor = entry.getKey();
+ final PropertyConfiguration configuration = entry.getValue();
+
+ // Determine if this property is affected by the Parameter Update
+ boolean propertyAffected = false;
+ final List<ParameterReference> parameterReferences = configuration.getParameterReferences();
+ for (final ParameterReference reference : parameterReferences) {
+ final String referencedParamName = reference.getParameterName();
+ if (updatedParameters.containsKey(referencedParamName)) {
+ propertyAffected = true;
+ componentAffected = true;
+ break;
+ }
+ }
+
+ if (propertyAffected) {
+ final String previousValue = configuration.getEffectiveValue(previousParameterLookup);
+ final String updatedValue = configuration.getEffectiveValue(getParameterContext());
+
+ // Check if the value of the property is truly affected. It's possible that we could have a property configured as something like "#{a}#{b}"
+ // Where parameter a = "abc-" and b = "cba". The update could change a to "abc" and b to "-cba". As a result, the property value previously was "abc-cba" and still is.
+ // In such a case, we should not call onPropertyModified.
+ final boolean propertyUpdated = !Objects.equals(previousValue, updatedValue);
+ if (propertyUpdated) {
+ try {
+ logger.debug("Parameter Context updated, resulting in property {} of {} changing. Calling onPropertyModified().", propertyDescriptor, this);
Review comment:
You're right... I looked at it too quickly and assumed that the line below was containing the parameters of the log statement. Doh.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services