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 2021/03/10 09:22:46 UTC
[GitHub] [nifi-minifi-cpp] fgerlits opened a new pull request #1025: MINIFICPP-1487 Add exponential backoff on failed flow files
fgerlits opened a new pull request #1025:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1025
#1013 had a bug; see https://issues.apache.org/jira/browse/MINIFICPP-1487?focusedCommentId=17297967
In this PR, I changed the duration of repeated flow file penalties to be double of the previous one, up to a limit of 1000 times the original. This will ensure that we won't always retry the same faulty flow file after an exception thrown in `Processor::onTrigger()`.
---
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] 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?
- [ ] If applicable, have you updated the NOTICE file?
### 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 GitHub Actions CI results 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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1025: MINIFICPP-1487 Add exponential backoff on failed flow files
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1025:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1025#discussion_r592156442
##########
File path: libminifi/include/core/ProcessorConfig.h
##########
@@ -35,10 +35,8 @@ namespace core {
#define DEFAULT_SCHEDULING_PERIOD_MILLIS 1000
#define DEFAULT_RUN_DURATION 0
#define DEFAULT_MAX_CONCURRENT_TASKS 1
-#define DEFAULT_PENALIZATION_PERIOD 1
-// Default yield period in second
#define DEFAULT_YIELD_PERIOD_SECONDS 1
-#define DEFAULT_PENALIZATION_PERIOD_SECONDS 30
+constexpr std::chrono::seconds DEFAULT_PENALIZATION_PERIOD{30};
Review comment:
done: https://github.com/apache/nifi-minifi-cpp/pull/1025/commits/38c9df55ef1f8e4767cb04147273be6c3ba39d8a and https://github.com/apache/nifi-minifi-cpp/pull/1025/commits/ca6d778860c3f01e54d1dff64a300600a1108941
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1025: MINIFICPP-1487 Add exponential backoff on failed flow files
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1025:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1025#discussion_r592148641
##########
File path: libminifi/include/core/state/nodes/SchedulingNodes.h
##########
@@ -85,7 +85,7 @@ class SchedulingDefaults : public DeviceInformation {
SerializedResponseNode penalizationPeriod;
penalizationPeriod.name = "penalizationPeriodMillis";
- penalizationPeriod.value = DEFAULT_PENALIZATION_PERIOD_SECONDS*1000;
+ penalizationPeriod.value = std::chrono::duration_cast<std::chrono::milliseconds>(core::DEFAULT_PENALIZATION_PERIOD).count();
Review comment:
I'm happy to change this, especially since it's shorter, which is always a good thing, but why is using the constructor better? They compile to the same thing (at least in this small example): https://godbolt.org/z/nh37j1
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1025: MINIFICPP-1487 Add exponential backoff on failed flow files
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1025:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1025#discussion_r592157012
##########
File path: libminifi/include/core/state/nodes/SchedulingNodes.h
##########
@@ -85,7 +85,7 @@ class SchedulingDefaults : public DeviceInformation {
SerializedResponseNode penalizationPeriod;
penalizationPeriod.name = "penalizationPeriodMillis";
- penalizationPeriod.value = DEFAULT_PENALIZATION_PERIOD_SECONDS*1000;
+ penalizationPeriod.value = std::chrono::duration_cast<std::chrono::milliseconds>(core::DEFAULT_PENALIZATION_PERIOD).count();
Review comment:
done in https://github.com/apache/nifi-minifi-cpp/pull/1025/commits/c5a16553add98628476bf642a83ea4f62ac222f5
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on pull request #1025: MINIFICPP-1487 Add exponential backoff on failed flow files
Posted by GitBox <gi...@apache.org>.
fgerlits commented on pull request #1025:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1025#issuecomment-796786719
Cancelling this in favor of #1027, which fixes the same bug, but is simpler and less likely to have unintended consequences.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on pull request #1025: MINIFICPP-1487 Add exponential backoff on failed flow files
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on pull request #1025:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1025#issuecomment-795292511
Shouldn't we document this behaviour somewhere?
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1025: MINIFICPP-1487 Add exponential backoff on failed flow files
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1025:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1025#discussion_r592311005
##########
File path: libminifi/include/core/state/nodes/SchedulingNodes.h
##########
@@ -85,7 +85,7 @@ class SchedulingDefaults : public DeviceInformation {
SerializedResponseNode penalizationPeriod;
penalizationPeriod.name = "penalizationPeriodMillis";
- penalizationPeriod.value = DEFAULT_PENALIZATION_PERIOD_SECONDS*1000;
+ penalizationPeriod.value = std::chrono::duration_cast<std::chrono::milliseconds>(core::DEFAULT_PENALIZATION_PERIOD).count();
Review comment:
I think `duration_cast` is mainly meant for lossy conversion while the implicit constructors make it clear that no precision loss occurs.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1025: MINIFICPP-1487 Add exponential backoff on failed flow files
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1025:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1025#discussion_r592172979
##########
File path: libminifi/include/core/ProcessorConfig.h
##########
@@ -35,10 +35,8 @@ namespace core {
#define DEFAULT_SCHEDULING_PERIOD_MILLIS 1000
#define DEFAULT_RUN_DURATION 0
#define DEFAULT_MAX_CONCURRENT_TASKS 1
-#define DEFAULT_PENALIZATION_PERIOD 1
-// Default yield period in second
#define DEFAULT_YIELD_PERIOD_SECONDS 1
-#define DEFAULT_PENALIZATION_PERIOD_SECONDS 30
+constexpr std::chrono::seconds DEFAULT_PENALIZATION_PERIOD{30};
Review comment:
Although this means, at least without #1027, that in case of a faulty flow file causing a rollback (and an administrative yield of 30s), we'll have
* T+0 - rollback, yield for 30s, penalize the flow file 5s
* T+30s - retry the same faulty flow file, rollback, yield for 30s, penalize the flow file 10s
* T+60s - retry the same faulty flow file, rollback, yield for 30s, penalize the flow file 20s
* T+90s - retry the same faulty flow file, rollback, yield for 30s, penalize the flow file 40s
* T+120s - pick up the next non-penalized flow file from the queue
So the faulty flow file would hold up processing for 2 minutes. Do you want to reduce the administrative yield to 5 seconds, too? Or we could merge #1027, which would solve this problem in another way.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #1025: MINIFICPP-1487 Add exponential backoff on failed flow files
Posted by GitBox <gi...@apache.org>.
arpadboda commented on a change in pull request #1025:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1025#discussion_r592357858
##########
File path: libminifi/include/core/ProcessorConfig.h
##########
@@ -35,10 +35,8 @@ namespace core {
#define DEFAULT_SCHEDULING_PERIOD_MILLIS 1000
#define DEFAULT_RUN_DURATION 0
#define DEFAULT_MAX_CONCURRENT_TASKS 1
-#define DEFAULT_PENALIZATION_PERIOD 1
-// Default yield period in second
#define DEFAULT_YIELD_PERIOD_SECONDS 1
-#define DEFAULT_PENALIZATION_PERIOD_SECONDS 30
+constexpr std::chrono::seconds DEFAULT_PENALIZATION_PERIOD{30};
Review comment:
I agree with the suggestion of @fgerlits , the yield time of processor in case of exception should be less than the FF penalty.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1025: MINIFICPP-1487 Add exponential backoff on failed flow files
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1025:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1025#discussion_r591433099
##########
File path: libminifi/include/core/ProcessorConfig.h
##########
@@ -35,10 +35,8 @@ namespace core {
#define DEFAULT_SCHEDULING_PERIOD_MILLIS 1000
#define DEFAULT_RUN_DURATION 0
#define DEFAULT_MAX_CONCURRENT_TASKS 1
-#define DEFAULT_PENALIZATION_PERIOD 1
-// Default yield period in second
#define DEFAULT_YIELD_PERIOD_SECONDS 1
-#define DEFAULT_PENALIZATION_PERIOD_SECONDS 30
+constexpr std::chrono::seconds DEFAULT_PENALIZATION_PERIOD{30};
Review comment:
With exponential backoff now implemented, I think it would make sense to reduce the default penalty. I would go for 5 sec. With a max multiplier of 1000, this would mean a max penalty of more than an hour, which is plenty IMO.
##########
File path: libminifi/include/core/state/nodes/SchedulingNodes.h
##########
@@ -85,7 +85,7 @@ class SchedulingDefaults : public DeviceInformation {
SerializedResponseNode penalizationPeriod;
penalizationPeriod.name = "penalizationPeriodMillis";
- penalizationPeriod.value = DEFAULT_PENALIZATION_PERIOD_SECONDS*1000;
+ penalizationPeriod.value = std::chrono::duration_cast<std::chrono::milliseconds>(core::DEFAULT_PENALIZATION_PERIOD).count();
Review comment:
Instead of duration cast, we can use the duration constructor for lossless conversion.
```suggestion
penalizationPeriod.value = std::chrono::milliseconds{core::DEFAULT_PENALIZATION_PERIOD}.count();
```
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1025: MINIFICPP-1487 Add exponential backoff on failed flow files
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1025:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1025#discussion_r592311005
##########
File path: libminifi/include/core/state/nodes/SchedulingNodes.h
##########
@@ -85,7 +85,7 @@ class SchedulingDefaults : public DeviceInformation {
SerializedResponseNode penalizationPeriod;
penalizationPeriod.name = "penalizationPeriodMillis";
- penalizationPeriod.value = DEFAULT_PENALIZATION_PERIOD_SECONDS*1000;
+ penalizationPeriod.value = std::chrono::duration_cast<std::chrono::milliseconds>(core::DEFAULT_PENALIZATION_PERIOD).count();
Review comment:
I think of `duration_cast` is mainly meant for lossy conversion while the implicit constructors make it clear that no precision loss occurs.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] fgerlits closed pull request #1025: MINIFICPP-1487 Add exponential backoff on failed flow files
Posted by GitBox <gi...@apache.org>.
fgerlits closed pull request #1025:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1025
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on pull request #1025: MINIFICPP-1487 Add exponential backoff on failed flow files
Posted by GitBox <gi...@apache.org>.
fgerlits commented on pull request #1025:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1025#issuecomment-796554633
> Shouldn't we document this behaviour somewhere?
Yes, good idea. (Assuming this PR gets merged; it's more of a proposal for now.) Do you have a suggestion where to document 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1025: MINIFICPP-1487 Add exponential backoff on failed flow files
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1025:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1025#discussion_r592309864
##########
File path: libminifi/include/core/ProcessorConfig.h
##########
@@ -35,10 +35,8 @@ namespace core {
#define DEFAULT_SCHEDULING_PERIOD_MILLIS 1000
#define DEFAULT_RUN_DURATION 0
#define DEFAULT_MAX_CONCURRENT_TASKS 1
-#define DEFAULT_PENALIZATION_PERIOD 1
-// Default yield period in second
#define DEFAULT_YIELD_PERIOD_SECONDS 1
-#define DEFAULT_PENALIZATION_PERIOD_SECONDS 30
+constexpr std::chrono::seconds DEFAULT_PENALIZATION_PERIOD{30};
Review comment:
I'm not sure about that. What do you think @arpadboda ?
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1025: MINIFICPP-1487 Add exponential backoff on failed flow files
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1025:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1025#discussion_r592156442
##########
File path: libminifi/include/core/ProcessorConfig.h
##########
@@ -35,10 +35,8 @@ namespace core {
#define DEFAULT_SCHEDULING_PERIOD_MILLIS 1000
#define DEFAULT_RUN_DURATION 0
#define DEFAULT_MAX_CONCURRENT_TASKS 1
-#define DEFAULT_PENALIZATION_PERIOD 1
-// Default yield period in second
#define DEFAULT_YIELD_PERIOD_SECONDS 1
-#define DEFAULT_PENALIZATION_PERIOD_SECONDS 30
+constexpr std::chrono::seconds DEFAULT_PENALIZATION_PERIOD{30};
Review comment:
done: https://github.com/apache/nifi-minifi-cpp/pull/1025/commits/38c9df55ef1f8e4767cb04147273be6c3ba39d8a
----------------------------------------------------------------
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