MINIFICPP-1273 - Drain connections on flow shutdown by adamdebreceni · Pull Request #827 · apache/nifi-minifi-cpp

adamdebreceni · 2020-06-30T09:28:48Z

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?
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.

szaszm

only minor issues, your changes look reasonable

szaszm · 2020-06-30T09:46:17Z

+  for (std::set<ProcessGroup *>::iterator it = child_process_groups_.begin(); it != child_process_groups_.end(); ++it) {
+    (*it)->drainConnections();
+  }


This could become a range-based for loop.

szaszm · 2020-06-30T09:48:27Z

+    MATH(EXPR FLOW_TEST_COUNT "${FLOW_TEST_COUNT}+1")
+    add_test(NAME "${testfilename}" COMMAND "${testfilename}" WORKING_DIRECTORY ${TEST_DIR})
+ENDFOREACH()
+message("-- Finished building ${FLOW_TEST_COUNT} flow related test file(s)...")


The file should end with a newline character.

https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline

(it seems like the Enter puts too much stress on my pinky)

(some more bikeshedding questions about the POSIX "line")

what does POSIX say about where the cursor would be? it cannot be on line 2 as there is no line 2

or take the error message:

what is the 2 in test.cpp:2:8?

we could have "the line n" mean the set of characters after the (n-1)th line up to the next \n, but that seems like a huge stretch

The tools were written to behave decently with non-text files and consider the line number to be the encountered \ns + 1. Historical posix text processing utilities may ignore the contents after the last \n, but I haven't seen modern tools do so. [1]

Please note that I'm by no means a POSIX lawyer, and the part about handling is mostly speculation.

[1] https://unix.stackexchange.com/a/18789/366131

I'm not sure about the above implication (\n before EOF -> is text file), but its contrary (no \n before EOF -> is not a text file) seems to be true to me.

this is a response to a deleted comment:
text file is a file format that has the magic number \n at the end of the file 🤯

(I don't know how to resurrect the comment)
(although I didn't mean to imply that \n before EOF -> is text file, e.g. having a file with only the magic number won't make a file a zip file)

arpadboda

Looks good to me, too, one minor comment and a question.

arpadboda · 2020-06-30T15:59:34Z

  }

-  for (auto &&connection : connections_) {
+  for (auto&& connection : connections_) {


Why rvalue ref here?

That's a forwarding reference, because auto uses template deduction rules. In other words, "I don't care what it is, bind a reference to it" reference.
In this case it will be a const lvalue reference, because:

auto&&

const std::shared_ptr<Connection>& && after deducing auto (note: connections_ is a std::set, which only has const iterators)

const std::shared_ptr<Connection>& after reference-collapsing

Normally I point out that const auto& or auto& would be more explicit and readable, but since this is old code with just a space change, I didn't want to bother @adamdebreceni with this.

I am torn between always use auto&& in range-based for loops and never, it is really only useful when the iteration yields a temporary proxy object like with std::vector<bool>, auto& could be used, I wouldn't use const auto& as it makes me think that the connection is const whereas only the std::shared_ptr<...> is

arpadboda · 2020-06-30T16:02:00Z

+#include "../TestBase.h"
+#include "YamlConfiguration.h"
+
+const char* yamlConfig =


We have a test/resources folder, where you could place it as a real yaml file and read it.

The location of the yaml file can be a cmd line argument as well as we do in our integration tests.

Edit: passing command line args to ctest won't work, please ignore this comment

I saw that a number of tests store their config files there, but the fact that I have to search for the test by name then check the CMakeLists.txt to find out what the command line arguments are going to be, and then manually navigate to the file, makes me think that it is not the best approach

szaszm reviewed Jun 30, 2020

View reviewed changes

arpadboda reviewed Jun 30, 2020

View reviewed changes

arpadboda approved these changes Jun 30, 2020

View reviewed changes

adebreceni added 3 commits July 1, 2020 13:50

MINIFICPP-1273 - Drain connections on flow shutdown

0e389d4

MINIFICPP-1273 - Review changes

a7bc939

MINIFICPP-1273 - Preempt bug

c21e10b

adamdebreceni force-pushed the MINIFICPP-1273 branch from ddbcd74 to c21e10b Compare July 1, 2020 11:50

szaszm approved these changes Jul 7, 2020

View reviewed changes

arpadboda closed this in ff785ce Jul 7, 2020

Conversation

adamdebreceni commented Jun 30, 2020

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

szaszm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szaszm Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamdebreceni Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamdebreceni Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arpadboda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arpadboda Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamdebreceni Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

szaszm Jun 30, 2020 •

edited

Loading

adamdebreceni Jul 6, 2020 •

edited

Loading

adamdebreceni Jul 6, 2020 •

edited

Loading

arpadboda Jun 30, 2020 •

edited

Loading

adamdebreceni Jul 1, 2020 •

edited

Loading