You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "vuong2407 (via GitHub)" <gi...@apache.org> on 2023/11/24 10:54:06 UTC
[PR] NIFI-12402 add option to user can avoid an inactive indicator is crea… [nifi]
vuong2407 opened a new pull request, #8063:
URL: https://github.com/apache/nifi/pull/8063
…ted even if there is no actual activity when processor is started
<!-- 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. -->
# Summary
[NIFI-12402](https://issues.apache.org/jira/browse/NIFI-12402)
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [x] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [x] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [x] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [x] Pull Request based on current revision of the `main` branch
- [x] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [x] Build completed using `mvn clean install -P contrib-check`
- [x] JDK 21
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12402 add option to user can avoid an inactive indicator is crea… [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #8063:
URL: https://github.com/apache/nifi/pull/8063#discussion_r1454401906
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestMonitorActivity.java:
##########
@@ -349,6 +408,50 @@ public void testFirstRunNoMessages() throws InterruptedException {
while(rerun);
}
+ @Test
+ public void testRunNoMessagesWithWaitForActivityTrue() throws InterruptedException {
+ // don't use the TestableProcessor, we want the real timestamp from @OnScheduled
+ final TestRunner runner = TestRunners.newTestRunner(new MonitorActivity());
+ runner.setProperty(MonitorActivity.CONTINUALLY_SEND_MESSAGES, "false");
+ runner.setProperty(MonitorActivity.THRESHOLD, "5 secs");
+ runner.setProperty(MonitorActivity.WAIT_FOR_ACTIVITY, "true");
+
+ // shouldn't generate inactivity b/c run() will reset the lastSuccessfulTransfer if @OnSchedule & onTrigger
+ // does not get called more than MonitorActivity.THRESHOLD apart
+ runner.run();
+ runner.assertTransferCount(MonitorActivity.REL_SUCCESS, 0);
+ runner.assertTransferCount(MonitorActivity.REL_INACTIVE, 0);
+ runner.assertTransferCount(MonitorActivity.REL_ACTIVITY_RESTORED, 0);
+ Thread.sleep(10000L);
+ runNext(runner);
+ runner.assertTransferCount(MonitorActivity.REL_SUCCESS, 0);
+ runner.assertTransferCount(MonitorActivity.REL_INACTIVE, 0);
+ runner.assertTransferCount(MonitorActivity.REL_ACTIVITY_RESTORED, 0);
+ runner.clearTransferState();
+ }
+
+ @Test
+ public void testRunNoMessagesWithWaitForActivityFalse() throws InterruptedException {
+ // don't use the TestableProcessor, we want the real timestamp from @OnScheduled
+ final TestRunner runner = TestRunners.newTestRunner(new MonitorActivity());
+ runner.setProperty(MonitorActivity.CONTINUALLY_SEND_MESSAGES, "false");
+ runner.setProperty(MonitorActivity.THRESHOLD, "5 secs");
+ runner.setProperty(MonitorActivity.WAIT_FOR_ACTIVITY, "false");
+
+ // shouldn't generate inactivity b/c run() will reset the lastSuccessfulTransfer if @OnSchedule & onTrigger
+ // does not get called more than MonitorActivity.THRESHOLD apart
+ runner.run();
+ runner.assertTransferCount(MonitorActivity.REL_SUCCESS, 0);
+ runner.assertTransferCount(MonitorActivity.REL_INACTIVE, 0);
+ runner.assertTransferCount(MonitorActivity.REL_ACTIVITY_RESTORED, 0);
+ Thread.sleep(10000L);
+ runNext(runner);
+ runner.assertTransferCount(MonitorActivity.REL_SUCCESS, 0);
+ runner.assertTransferCount(MonitorActivity.REL_INACTIVE, 1);
+ runner.assertTransferCount(MonitorActivity.REL_ACTIVITY_RESTORED, 0);
+ runner.clearTransferState();
+ }
+
Review Comment:
The sleep of 10 seconds is too long for efficient unit testing. The other unit test method provides some basic coverage, so recommend removing these two test methods.
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestMonitorActivity.java:
##########
@@ -100,6 +100,65 @@ public void testFirstMessage() {
restoredFlowFile.assertAttributeNotExists("key1");
}
+ @Test
+ public void testFirstMessageWithWaitForActivityTrue() {
+ final TestableProcessor processor = new TestableProcessor(1000);
+ final TestRunner runner = TestRunners.newTestRunner(processor);
+ runner.setProperty(MonitorActivity.CONTINUALLY_SEND_MESSAGES, "false");
+ runner.setProperty(MonitorActivity.THRESHOLD, "100 millis");
+ runner.setProperty(MonitorActivity.WAIT_FOR_ACTIVITY, "true");
+
+ runner.enqueue(new byte[0]);
+ runner.run();
+ runner.assertAllFlowFilesTransferred(MonitorActivity.REL_SUCCESS, 1);
+ runner.clearTransferState();
+
+ processor.resetLastSuccessfulTransfer();
+
+ runNext(runner);
+ runner.assertAllFlowFilesTransferred(MonitorActivity.REL_INACTIVE, 1);
+ runner.clearTransferState();
+
+ Map<String, String> attributes = new HashMap<>();
+ attributes.put("key", "value");
+ attributes.put("key1", "value1");
+
+ runner.enqueue(new byte[0], attributes);
+ runNext(runner);
+
+ runner.assertTransferCount(MonitorActivity.REL_SUCCESS, 1);
+ runner.assertTransferCount(MonitorActivity.REL_ACTIVITY_RESTORED, 1);
+
+ MockFlowFile restoredFlowFile = runner.getFlowFilesForRelationship(MonitorActivity.REL_ACTIVITY_RESTORED).get(0);
+ String flowFileContent = new String(restoredFlowFile.toByteArray());
+ assertTrue(Pattern.matches("Activity restored at time: (.*) after being inactive for 0 minutes", flowFileContent));
Review Comment:
It is best to avoid checking particular messages, as these are not necessarily part of the component's basic behavior. Recommend removing this assertion and the similar one at the end of the method.
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12402 add option to user can avoid an inactive indicator is crea… [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #8063:
URL: https://github.com/apache/nifi/pull/8063#discussion_r1452722401
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/MonitorActivity.java:
##########
@@ -105,6 +105,14 @@ public class MonitorActivity extends AbstractProcessor {
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.defaultValue("Activity restored at time: ${now():format('yyyy/MM/dd HH:mm:ss')} after being inactive for ${inactivityDurationMillis:toNumber():divide(60000)} minutes")
.build();
+ public static final PropertyDescriptor WAIT_FOR_ACTIVITY = new PropertyDescriptor.Builder()
+ .name("Wait For Activity")
Review Comment:
Following the general Title Case convention for property names, recommend adjusting `For` to `for`.
```suggestion
.name("Wait for Activity")
```
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/MonitorActivity.java:
##########
@@ -105,6 +105,14 @@ public class MonitorActivity extends AbstractProcessor {
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.defaultValue("Activity restored at time: ${now():format('yyyy/MM/dd HH:mm:ss')} after being inactive for ${inactivityDurationMillis:toNumber():divide(60000)} minutes")
.build();
+ public static final PropertyDescriptor WAIT_FOR_ACTIVITY = new PropertyDescriptor.Builder()
+ .name("Wait For Activity")
+ .description("When the processor gets started or restarted, if set to true, only send an inactive indicator if there had been activity beforehand. "
+ + "Otherwise send an inactive indicator even if there had not been activity beforehand.")
+ .required(false)
Review Comment:
Since this property has a default value that preserves the existing behavior, this new property can be marked as required.
```suggestion
.required(true)
```
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12402 add option to user can avoid an inactive indicator is crea… [nifi]
Posted by "dan-s1 (via GitHub)" <gi...@apache.org>.
dan-s1 commented on code in PR #8063:
URL: https://github.com/apache/nifi/pull/8063#discussion_r1431617697
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/MonitorActivity.java:
##########
@@ -105,6 +105,14 @@ public class MonitorActivity extends AbstractProcessor {
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.defaultValue("Activity restored at time: ${now():format('yyyy/MM/dd HH:mm:ss')} after being inactive for ${inactivityDurationMillis:toNumber():divide(60000)} minutes")
.build();
+ public static final PropertyDescriptor WAIT_FOR_ACTIVITY = new PropertyDescriptor.Builder()
+ .name("Initial Inactivity Indicator")
+ .description("When the processor gets started or restarted, it is considered there was no activity happened before. "
+ + "If true, send inactivity indicator only if there was activity.")
Review Comment:
```suggestion
.description("When the processor gets started or restarted, if set to true, only send an inactive indicator if there had been activity beforehand. Otherwise send an inactive indicator even if there had not been activity beforehand.")
```
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12402 add option to user can avoid an inactive indicator is crea… [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory closed pull request #8063: NIFI-12402 add option to user can avoid an inactive indicator is crea…
URL: https://github.com/apache/nifi/pull/8063
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12402 add option to user can avoid an inactive indicator is crea… [nifi]
Posted by "dan-s1 (via GitHub)" <gi...@apache.org>.
dan-s1 commented on code in PR #8063:
URL: https://github.com/apache/nifi/pull/8063#discussion_r1428029603
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/MonitorActivity.java:
##########
@@ -300,7 +312,11 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
if (isInactive) {
final boolean continual = context.getProperty(CONTINUALLY_SEND_MESSAGES).asBoolean();
- sendInactiveMarker = !inactive.getAndSet(true) || (continual && (now > lastInactiveMessage.get() + thresholdMillis));
+ if (!initialInactivityIndicator) {
+ sendInactiveMarker = (!inactive.getAndSet(true) || (continual && (now > lastInactiveMessage.get() + thresholdMillis))) && hasSuccessTransfer.get();
+ } else {
+ sendInactiveMarker = !inactive.getAndSet(true) || (continual && (now > lastInactiveMessage.get() + thresholdMillis));
+ }
Review Comment:
`!inactive.getAndSet(true) || (continual && (now > lastInactiveMessage.get()` is common code between lines 316 and 318. Can you please assign this to a variable whose name should describe what these conditions are and then use the variable on lines 316 and 318? Thanks!
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12402 add option to user can avoid an inactive indicator is crea… [nifi]
Posted by "dan-s1 (via GitHub)" <gi...@apache.org>.
dan-s1 commented on PR #8063:
URL: https://github.com/apache/nifi/pull/8063#issuecomment-1865110606
LGTM +1
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12402 add option to user can avoid an inactive indicator is crea… [nifi]
Posted by "nathluu (via GitHub)" <gi...@apache.org>.
nathluu commented on PR #8063:
URL: https://github.com/apache/nifi/pull/8063#issuecomment-1858787407
Hi @dan-s1,
Codes were updated. Please help to review it again.
Thank you!
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org