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/08/04 11:58:48 UTC
[GitHub] [nifi-minifi-cpp] adamdebreceni opened a new pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
adamdebreceni opened a new pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861
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:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] 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.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [ ] 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 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
[GitHub] [nifi-minifi-cpp] szaszm commented on pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
szaszm commented on pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#issuecomment-672770473
Merged in 53ba922, but forgot to link the PR in the commit.
----------------------------------------------------------------
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 #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465088044
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", "[TestFlow2]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
Review comment:
I would prefer a separate additional function (possibly with both the old and the new function calling a common implementation).
----------------------------------------------------------------
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 #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r466255118
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -79,6 +79,17 @@ Flow Controller:
Remote Processing Groups:
)";
+template<typename Fn>
+bool verifyWithBusyWait(std::chrono::milliseconds timeout, Fn&& fn) {
+ auto start = std::chrono::steady_clock::now();
+ while (std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::steady_clock::now() - start) < timeout) {
Review comment:
minor, but I don't think you need the `duration_cast`: durations in different units can be compared
----------------------------------------------------------------
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] adamdebreceni commented on a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465039735
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", "[TestFlow2]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
- REQUIRE(root->getTotalFlowFileCount() == 3);
+ REQUIRE(root->getTotalFlowFileCount() != 0);
Review comment:
removed the superfluous checks
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -215,20 +220,19 @@ TEST_CASE("Extend the waiting period during shutdown", "[TestFlow4]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
- REQUIRE(root->getTotalFlowFileCount() == 3);
+ REQUIRE(root->getTotalFlowFileCount() != 0);
REQUIRE(sourceProc->trigger_count.load() == 1);
std::thread shutdownThread([&]{
execSinkPromise.set_value();
controller->stop(true);
});
- while (controller->isRunning()) {
+ int extendCount = 0;
+ while (extendCount++ < 5 && controller->isRunning()) {
+ testController.logger_->log_info("Controller still running, extend the waiting period, ff count: %d", static_cast<int>(root->getTotalFlowFileCount()));
Review comment:
added
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", "[TestFlow2]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
- REQUIRE(root->getTotalFlowFileCount() == 3);
+ REQUIRE(root->getTotalFlowFileCount() != 0);
Review comment:
removed
----------------------------------------------------------------
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] adamdebreceni commented on a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465028033
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", "[TestFlow2]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
Review comment:
only seemingly weaker, since the `3` flowFiles are generated in batch, and their insertion is one atomic operation
----------------------------------------------------------------
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] adamdebreceni commented on a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465030783
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", "[TestFlow2]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
Review comment:
the `sleep_for` inside the `verifyEventHappenedInPollTime` is problematic, as I would like to wait for the sourceProcessor to get executed once, and as it turns out a on the github agents a `sleep_for(20ms)` might happily wait more than `100ms`
(note that I can't signal the main thread from the `onTrigger` as the shutdown logic checks the actual contents of the connections, so if I signal from the `onTrigger` and initiate a flowController shutdown the generated flowFiles might not yet have been enqueued and do not trigger the graceful shutdown)
----------------------------------------------------------------
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 #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465013502
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", "[TestFlow2]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
- REQUIRE(root->getTotalFlowFileCount() == 3);
+ REQUIRE(root->getTotalFlowFileCount() != 0);
Review comment:
why do we need this REQUIRE? `busy_wait` (or `verifyEventHappenedInPollTime` if you change to that) already checked this condition in the previous line
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -215,20 +220,19 @@ TEST_CASE("Extend the waiting period during shutdown", "[TestFlow4]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
- REQUIRE(root->getTotalFlowFileCount() == 3);
+ REQUIRE(root->getTotalFlowFileCount() != 0);
REQUIRE(sourceProc->trigger_count.load() == 1);
std::thread shutdownThread([&]{
execSinkPromise.set_value();
controller->stop(true);
});
- while (controller->isRunning()) {
+ int extendCount = 0;
+ while (extendCount++ < 5 && controller->isRunning()) {
+ testController.logger_->log_info("Controller still running, extend the waiting period, ff count: %d", static_cast<int>(root->getTotalFlowFileCount()));
Review comment:
it would be useful to include the value of `extendCount ` in this log
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", "[TestFlow2]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
Review comment:
You could use `verifyEventHappenedInPollTime` in IntegrationTestUtils.h, it is almost the same as `busy_wait`.
```suggestion
REQUIRE(verifyEventHappenedInPollTime(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;}));
```
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", "[TestFlow2]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
Review comment:
Is there no way to avoid changing `== 3` to `!= 0`? The test condition is much weaker now.
----------------------------------------------------------------
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 a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465018749
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -215,20 +220,19 @@ TEST_CASE("Extend the waiting period during shutdown", "[TestFlow4]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
- REQUIRE(root->getTotalFlowFileCount() == 3);
+ REQUIRE(root->getTotalFlowFileCount() != 0);
REQUIRE(sourceProc->trigger_count.load() == 1);
std::thread shutdownThread([&]{
execSinkPromise.set_value();
controller->stop(true);
});
- while (controller->isRunning()) {
+ int extendCount = 0;
+ while (extendCount++ < 5 && controller->isRunning()) {
Review comment:
Maybe we could have more attempts with smaller timeout increases.
----------------------------------------------------------------
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] adamdebreceni commented on a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r466186234
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -215,20 +220,19 @@ TEST_CASE("Extend the waiting period during shutdown", "[TestFlow4]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
- REQUIRE(root->getTotalFlowFileCount() == 3);
+ REQUIRE(root->getTotalFlowFileCount() != 0);
REQUIRE(sourceProc->trigger_count.load() == 1);
std::thread shutdownThread([&]{
execSinkPromise.set_value();
controller->stop(true);
});
- while (controller->isRunning()) {
+ int extendCount = 0;
+ while (extendCount++ < 5 && controller->isRunning()) {
Review comment:
note that here we are not simply waiting for the controller to shutdown, but we are verifying if we can progressively delay the shutdown by adjusting the timeout parameter, shorter sleeps seem to be relatively more imprecise than longer ones on the agents, I understand now that this might cause confusion, will add some check to better convey its purpose
----------------------------------------------------------------
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 a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465016666
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", "[TestFlow2]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
- REQUIRE(root->getTotalFlowFileCount() == 3);
+ REQUIRE(root->getTotalFlowFileCount() != 0);
Review comment:
Would we not have already thrown if this was not 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.
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 #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465615443
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", "[TestFlow2]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
Review comment:
@adebreceni It wasn't really a proposal, but I would prefer two separate functions `verifyEventHappenedInPollTime()` and `verifyEventHappenedInPollTimeWithBusyWait()` (just an example, feel free to find a shorter name) rather than a single function where an argument value of -1 means "use the busy version".
[TBH, I am not a fan of the `verifyEventHappenedInPollTime` name, as something like `REQUIRE(happensBeforeTimeout(...))` would read better, but renaming it is probably not in scope for this PR.]
----------------------------------------------------------------
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] adamdebreceni commented on a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465038647
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -215,20 +220,19 @@ TEST_CASE("Extend the waiting period during shutdown", "[TestFlow4]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
- REQUIRE(root->getTotalFlowFileCount() == 3);
+ REQUIRE(root->getTotalFlowFileCount() != 0);
REQUIRE(sourceProc->trigger_count.load() == 1);
std::thread shutdownThread([&]{
execSinkPromise.set_value();
controller->stop(true);
});
- while (controller->isRunning()) {
+ int extendCount = 0;
+ while (extendCount++ < 5 && controller->isRunning()) {
+ testController.logger_->log_info("Controller still running, extend the waiting period, ff count: %d", static_cast<int>(root->getTotalFlowFileCount()));
Review comment:
done
----------------------------------------------------------------
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] adamdebreceni commented on a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465039368
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -215,20 +220,19 @@ TEST_CASE("Extend the waiting period during shutdown", "[TestFlow4]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
- REQUIRE(root->getTotalFlowFileCount() == 3);
+ REQUIRE(root->getTotalFlowFileCount() != 0);
REQUIRE(sourceProc->trigger_count.load() == 1);
std::thread shutdownThread([&]{
execSinkPromise.set_value();
controller->stop(true);
});
- while (controller->isRunning()) {
+ int extendCount = 0;
+ while (extendCount++ < 5 && controller->isRunning()) {
Review comment:
the `sleep_for`, as it turns out, is not very precise especially on the github agents :(
----------------------------------------------------------------
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 #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465840051
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -215,20 +220,19 @@ TEST_CASE("Extend the waiting period during shutdown", "[TestFlow4]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
- REQUIRE(root->getTotalFlowFileCount() == 3);
+ REQUIRE(root->getTotalFlowFileCount() != 0);
REQUIRE(sourceProc->trigger_count.load() == 1);
std::thread shutdownThread([&]{
execSinkPromise.set_value();
controller->stop(true);
});
- while (controller->isRunning()) {
+ int extendCount = 0;
+ while (extendCount++ < 5 && controller->isRunning()) {
Review comment:
shorter sleeps and checking for a timeout instead of run count should achieve the same goal without multiplying the uncertainty of the agents.
----------------------------------------------------------------
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] adamdebreceni commented on a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465518803
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", "[TestFlow2]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
Review comment:
@hunyadi-dev I don't see why it should be compile-time decidable if we need sleep but alright will not add an extra parameter, @fgerlits could you expand on your proposal?
----------------------------------------------------------------
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] adamdebreceni commented on a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r466250542
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", "[TestFlow2]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
Review comment:
changed it to `verifyWithBusyWait`
----------------------------------------------------------------
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 closed pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
szaszm closed pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861
----------------------------------------------------------------
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 a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465020999
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -215,20 +220,19 @@ TEST_CASE("Extend the waiting period during shutdown", "[TestFlow4]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
- REQUIRE(root->getTotalFlowFileCount() == 3);
+ REQUIRE(root->getTotalFlowFileCount() != 0);
REQUIRE(sourceProc->trigger_count.load() == 1);
std::thread shutdownThread([&]{
execSinkPromise.set_value();
controller->stop(true);
});
- while (controller->isRunning()) {
+ int extendCount = 0;
+ while (extendCount++ < 5 && controller->isRunning()) {
+ testController.logger_->log_info("Controller still running, extend the waiting period, ff count: %d", static_cast<int>(root->getTotalFlowFileCount()));
Review comment:
Does `%u` not work here?
----------------------------------------------------------------
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 #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465088044
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", "[TestFlow2]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
Review comment:
I would prefer a function without a timeout parameter (possibly with both the timeout-y and timeout-less version calling a common implementation).
----------------------------------------------------------------
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] adamdebreceni commented on a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r466186234
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -215,20 +220,19 @@ TEST_CASE("Extend the waiting period during shutdown", "[TestFlow4]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
- REQUIRE(root->getTotalFlowFileCount() == 3);
+ REQUIRE(root->getTotalFlowFileCount() != 0);
REQUIRE(sourceProc->trigger_count.load() == 1);
std::thread shutdownThread([&]{
execSinkPromise.set_value();
controller->stop(true);
});
- while (controller->isRunning()) {
+ int extendCount = 0;
+ while (extendCount++ < 5 && controller->isRunning()) {
Review comment:
note that here we are not simply waiting for the controller to shutdown, but we are verifying if we can progressively delay the shutdown by adjusting the timeout parameter, shorter sleeps seem to be relatively more imprecise than longer ones on the agents, I understand now that this loop might not be the best so I will unroll it to properly convey its purpose
----------------------------------------------------------------
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 a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465508813
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", "[TestFlow2]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
Review comment:
I cannot see where to link points, but -1 from me for the additional argument as deducing if we need to sleep should be a compile-time operation.
----------------------------------------------------------------
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] adamdebreceni commented on a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465040140
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", "[TestFlow2]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
Review comment:
wrote back the `== 3` as it should make no difference
----------------------------------------------------------------
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 #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#issuecomment-668584035
I see I have been reviewing parallel to @fgerlits. Please mark the duplicates resolved :)
----------------------------------------------------------------
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 a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465019406
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", "[TestFlow2]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
Review comment:
The same applies to the other similar changes.
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", "[TestFlow2]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
- REQUIRE(root->getTotalFlowFileCount() == 3);
+ REQUIRE(root->getTotalFlowFileCount() != 0);
Review comment:
The same applies to the other similar changes.
----------------------------------------------------------------
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] adamdebreceni commented on a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465044242
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", "[TestFlow2]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
Review comment:
see [this](https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465030783), we could extend the `verifyEventHappenedInPollTime` with a `poll_interval_ms` where `-1` would indicate a busy checking, what do you think?
----------------------------------------------------------------
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 a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465016430
##########
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##########
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", "[TestFlow2]") {
testController.startFlow();
// wait for the source processor to enqueue its flowFiles
- int tryCount = 0;
- while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
- std::this_thread::sleep_for(std::chrono::milliseconds{20});
- }
+ busy_wait(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;});
Review comment:
Why not simply:
```c++
assert(verifyEventHappenedInPollTime(std::chrono::milliseconds{50}, [&] {return root->getTotalFlowFileCount() != 0;})
```
----------------------------------------------------------------
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