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 2022/11/09 04:10:55 UTC
[GitHub] [nifi] thenatog opened a new pull request, #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
thenatog opened a new pull request, #6638:
URL: https://github.com/apache/nifi/pull/6638
…dCount
* Knowing that this property/feature is planned to be deprecated, it still exists in NiFi 1.18.0 but currently doesn't work as expected. This PR should fix that for the time being before it's officially removed.
<!-- 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-10703](https://issues.apache.org/jira/browse/NIFI-10703)
# 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-10703) issue created
### Pull Request Tracking
- [x] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-10703`
- [x] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-10703`
### 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
- [ ] Build completed using `mvn clean install -P contrib-check`
- [ ] JDK 8
- [x] JDK 11
- [ ] JDK 17
### 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
[GitHub] [nifi] markobean commented on a diff in pull request #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
Posted by GitBox <gi...@apache.org>.
markobean commented on code in PR #6638:
URL: https://github.com/apache/nifi/pull/6638#discussion_r1023357713
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/serialization/VersionedFlowSynchronizer.java:
##########
@@ -378,6 +378,7 @@ private void synchronizeFlow(final FlowController controller, final DataFlow exi
if (versionedFlow != null) {
controller.setMaxTimerDrivenThreadCount(versionedFlow.getMaxTimerDrivenThreadCount());
+ controller.setMaxEventDrivenThreadCount(versionedFlow.getMaxTimerDrivenThreadCount());
Review Comment:
@thenatog It does feel a little hacky. If you don't set the value to 1 when declaring the variable, the default value of an int is 0. So you could use this in VersionedDataflow.java (line 58)
```
public int getMaxEventDrivenThreadCount() {
return maxEventDrivenThreadCount < 1 ? 1 : maxEventDrivenThreadCount;
}
```
Or even better:
```
public int getMaxEventDrivenThreadCount() {
return maxEventDrivenThreadCount < 1 ? DEFAULT_MAX_EVENT_DRIVEN_THREAD_COUNT : maxEventDrivenThreadCount;
}
```
.. and setup the proper `private static final` constant.
--
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
[GitHub] [nifi] markobean commented on a diff in pull request #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
Posted by GitBox <gi...@apache.org>.
markobean commented on code in PR #6638:
URL: https://github.com/apache/nifi/pull/6638#discussion_r1023237391
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/serialization/VersionedFlowSynchronizer.java:
##########
@@ -378,6 +378,7 @@ private void synchronizeFlow(final FlowController controller, final DataFlow exi
if (versionedFlow != null) {
controller.setMaxTimerDrivenThreadCount(versionedFlow.getMaxTimerDrivenThreadCount());
+ controller.setMaxEventDrivenThreadCount(versionedFlow.getMaxTimerDrivenThreadCount());
Review Comment:
This should be:
controller.setMaxEventDrivenThreadCount(versionedFlow.getMax**Event**DrivenThreadCount());
--
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
[GitHub] [nifi] thenatog commented on pull request #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
Posted by GitBox <gi...@apache.org>.
thenatog commented on PR #6638:
URL: https://github.com/apache/nifi/pull/6638#issuecomment-1315898396
Will address changes and push asap, 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
[GitHub] [nifi] markobean commented on a diff in pull request #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
Posted by GitBox <gi...@apache.org>.
markobean commented on code in PR #6638:
URL: https://github.com/apache/nifi/pull/6638#discussion_r1023357713
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/serialization/VersionedFlowSynchronizer.java:
##########
@@ -378,6 +378,7 @@ private void synchronizeFlow(final FlowController controller, final DataFlow exi
if (versionedFlow != null) {
controller.setMaxTimerDrivenThreadCount(versionedFlow.getMaxTimerDrivenThreadCount());
+ controller.setMaxEventDrivenThreadCount(versionedFlow.getMaxTimerDrivenThreadCount());
Review Comment:
It does feel a little hacky. If you don't set the value to 1 when declaring the variable, the default value of an int is 0. So you could use this in VersionedDataflow.java (line 58)
```
public int getMaxEventDrivenThreadCount() {
return maxEventDrivenThreadCount < 1 ? 1 : maxEventDrivenThreadCount;
}
}
```
Or even better:
```
public int getMaxEventDrivenThreadCount() {
return maxEventDrivenThreadCount < 1 ? DEFAULT_MAX_EVENT_DRIVEN_THREAD_COUNT : maxEventDrivenThreadCount;
}
```
.. and setup the proper `private static final` constant.
--
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
[GitHub] [nifi] mattyb149 commented on pull request #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on PR #6638:
URL: https://github.com/apache/nifi/pull/6638#issuecomment-1316183445
+1 LGTM, thanks for the review @markobean ! Merging to main
--
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
[GitHub] [nifi] markobean commented on a diff in pull request #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
Posted by GitBox <gi...@apache.org>.
markobean commented on code in PR #6638:
URL: https://github.com/apache/nifi/pull/6638#discussion_r1023357713
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/serialization/VersionedFlowSynchronizer.java:
##########
@@ -378,6 +378,7 @@ private void synchronizeFlow(final FlowController controller, final DataFlow exi
if (versionedFlow != null) {
controller.setMaxTimerDrivenThreadCount(versionedFlow.getMaxTimerDrivenThreadCount());
+ controller.setMaxEventDrivenThreadCount(versionedFlow.getMaxTimerDrivenThreadCount());
Review Comment:
@thenatog It does feel a little hacky. If you don't set the value to 1 when declaring the variable, the default value of an int is 0. So you could use this in VersionedDataflow.java (line 58)
```
public int getMaxEventDrivenThreadCount() {
return maxEventDrivenThreadCount < 1 ? 1 : maxEventDrivenThreadCount;
}
}
```
Or even better:
```
public int getMaxEventDrivenThreadCount() {
return maxEventDrivenThreadCount < 1 ? DEFAULT_MAX_EVENT_DRIVEN_THREAD_COUNT : maxEventDrivenThreadCount;
}
```
.. and setup the proper `private static final` constant.
--
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
[GitHub] [nifi] markobean commented on pull request #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
Posted by GitBox <gi...@apache.org>.
markobean commented on PR #6638:
URL: https://github.com/apache/nifi/pull/6638#issuecomment-1316060692
All looks good now. Built latest version (6 commits) and re-ran tests outlined above. NiFi started with only flow.json.gz (no flow.xml.gz) when the flow was taken from 1.18.0 (no maxEventDrivenThreadCount setting in flow.) Then, updating the value persisted through NiFi restart - with and without flow.xml.gz present.
Thanks @thenatog for the flurry of last-minute updates as we try to get this pushed out before the 1.19.0 RC starts.
+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
[GitHub] [nifi] mattyb149 closed pull request #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
Posted by GitBox <gi...@apache.org>.
mattyb149 closed pull request #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
URL: https://github.com/apache/nifi/pull/6638
--
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
[GitHub] [nifi] thenatog commented on a diff in pull request #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
Posted by GitBox <gi...@apache.org>.
thenatog commented on code in PR #6638:
URL: https://github.com/apache/nifi/pull/6638#discussion_r1023300403
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/serialization/VersionedFlowSynchronizer.java:
##########
@@ -378,6 +378,7 @@ private void synchronizeFlow(final FlowController controller, final DataFlow exi
if (versionedFlow != null) {
controller.setMaxTimerDrivenThreadCount(versionedFlow.getMaxTimerDrivenThreadCount());
+ controller.setMaxEventDrivenThreadCount(versionedFlow.getMaxTimerDrivenThreadCount());
Review Comment:
I updated and set the maxEventDrivenThreadCount = 1 in the VersionedDataflow. Wasn't sure if this was hacky or if there's a better place to set that default.
--
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
[GitHub] [nifi] markobean commented on pull request #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
Posted by GitBox <gi...@apache.org>.
markobean commented on PR #6638:
URL: https://github.com/apache/nifi/pull/6638#issuecomment-1315976123
The error message that resulted from the previous commit (missing max event thread count) was
```
Caused by: java.lang.IllegalArgumentException: Cannot set max number of threads to less than 2
at org.apache.nifi.controller.FlowController.setMaxThreadCount(FlowController.java:1594)
```
This is actually incorrect. The message should read "Cannot set max number of threads to less than 1". Please update message at FlowController.java:1594.
--
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
[GitHub] [nifi] markap14 commented on a diff in pull request #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
Posted by GitBox <gi...@apache.org>.
markap14 commented on code in PR #6638:
URL: https://github.com/apache/nifi/pull/6638#discussion_r1023238439
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/serialization/VersionedFlowSynchronizer.java:
##########
@@ -378,6 +378,7 @@ private void synchronizeFlow(final FlowController controller, final DataFlow exi
if (versionedFlow != null) {
controller.setMaxTimerDrivenThreadCount(versionedFlow.getMaxTimerDrivenThreadCount());
+ controller.setMaxEventDrivenThreadCount(versionedFlow.getMaxTimerDrivenThreadCount());
Review Comment:
The `getMaxEventDrivenThreadCount()` also will default to `0`, though. So if the flow.json.gz doesn't have that field (which will be everyone's when they are upgrading) it will call `controller.setMaxEventDrivenThreadCount(0)` which will throw an `IllegalArgumentException`. So need to either default the value to `1` or check if it's set before calling this.
--
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
[GitHub] [nifi] markobean commented on pull request #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
Posted by GitBox <gi...@apache.org>.
markobean commented on PR #6638:
URL: https://github.com/apache/nifi/pull/6638#issuecomment-1309351043
Performed a full build with Java 11 (basic build without contrib-check profile). Installed and started NiFi. The flow.json.gz file included a property for event driven threads with default value of 1:
` "maxEventDrivenThreadCount": 1,`
Changed the value to 5. Confirmed new value in UI and in flow.json.gz and flow.xml.gz.
```
"maxEventDrivenThreadCount": 5,
<maxEventDrivenThreadCount>5</maxEventDrivenThreadCount>
```
Restarted NiFi. The Maximum event driven thread count returned to "1" in the UI. Confirmed in flow.json.gz and flow.xml.gz.
```
"maxEventDrivenThreadCount": 1,
<maxEventDrivenThreadCount>1</maxEventDrivenThreadCount>
```
This PR does not fix the issue.
@thenatog Did you perform the above test? Did you get different results? I am about to be away for the long holiday weekend. If it is not resolved by next week, I'll take a deeper look to determine what is happening.
--
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
[GitHub] [nifi] markobean commented on pull request #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
Posted by GitBox <gi...@apache.org>.
markobean commented on PR #6638:
URL: https://github.com/apache/nifi/pull/6638#issuecomment-1309432249
I found the issue. I don't think the max event driven thread count was being read from the flow on startup.
See https://github.com/apache/nifi/blob/0643f336e8266043c4ec01e1c07b8ef5bb38b02a/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/serialization/VersionedFlowSynchronizer.java#L372
The following needs to be added:
```
controller.setMaxEventDrivenThreadCount(versionedFlow.getMaxEventDrivenThreadCount());
```
--
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
[GitHub] [nifi] thenatog commented on pull request #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
Posted by GitBox <gi...@apache.org>.
thenatog commented on PR #6638:
URL: https://github.com/apache/nifi/pull/6638#issuecomment-1315991857
I can update that log message. Yes as you say, the flow.xml.gz needs to be removed. Initially I thought the whole nifi.flow.configuration.file=./conf/flow.xml.gz property had to be removed from NiFi properties but that breaks things somewhat.
--
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
[GitHub] [nifi] thenatog commented on pull request #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
Posted by GitBox <gi...@apache.org>.
thenatog commented on PR #6638:
URL: https://github.com/apache/nifi/pull/6638#issuecomment-1310991448
My mistake, I saw that "maxEventDrivenThreadCount":1 was now being added to the flow.json and that it would be set to 10 when hitting Apply, but it seems to be reset on restart. I incorporated your change and found it now to work as expected. I've pushed the additional change.
--
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
[GitHub] [nifi] markap14 commented on a diff in pull request #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
Posted by GitBox <gi...@apache.org>.
markap14 commented on code in PR #6638:
URL: https://github.com/apache/nifi/pull/6638#discussion_r1023236412
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/serialization/VersionedFlowSynchronizer.java:
##########
@@ -378,6 +378,7 @@ private void synchronizeFlow(final FlowController controller, final DataFlow exi
if (versionedFlow != null) {
controller.setMaxTimerDrivenThreadCount(versionedFlow.getMaxTimerDrivenThreadCount());
+ controller.setMaxEventDrivenThreadCount(versionedFlow.getMaxTimerDrivenThreadCount());
Review Comment:
This should be `.getMaxEventDrivenThreadCount()` not `.getMaxTimerDrivenThreadCount()`
--
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
[GitHub] [nifi] markobean commented on a diff in pull request #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
Posted by GitBox <gi...@apache.org>.
markobean commented on code in PR #6638:
URL: https://github.com/apache/nifi/pull/6638#discussion_r1023240258
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/serialization/VersionedFlowSynchronizer.java:
##########
@@ -378,6 +378,7 @@ private void synchronizeFlow(final FlowController controller, final DataFlow exi
if (versionedFlow != null) {
controller.setMaxTimerDrivenThreadCount(versionedFlow.getMaxTimerDrivenThreadCount());
+ controller.setMaxEventDrivenThreadCount(versionedFlow.getMaxTimerDrivenThreadCount());
Review Comment:
yeah.. what @markap14 said. I missed his comment.
Would love to see this quickly fixed so it can be added to 1.19.0. 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
[GitHub] [nifi] markobean commented on pull request #6638: NIFI-10703 - Updated VersionedDataflow to support MaxEventDrivenThrea…
Posted by GitBox <gi...@apache.org>.
markobean commented on PR #6638:
URL: https://github.com/apache/nifi/pull/6638#issuecomment-1315988976
Other than above log message correction, this looks good now.
I tested by installing nifi and using the flow.json.gz from a NiFi 1.18.0 installation. There was only a `flow.json.gz` and no `flow.xml.gz` in the conf/ directory.
Started NiFi. No problem. `Maximum event driven thread count` defaulted to "1". The value was updated to "4" and NiFi restarted. The value was maintained. This proves the original symptom has been corrected.
Also, NiFi was shutdown, `flow.xml.gz` was removed, and NiFi started. Again, the value of "4" was maintained for `Maximum event driven thread count`.
The removal of `flow.xml.gz` was an important part of the process. If this version of the flow was present, then the event driven thread count was obtained from it.
--
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