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/12/14 15:30:16 UTC

[GitHub] [nifi-minifi-cpp] lordgamez opened a new pull request #959: MINIFICPP-1424 Stabilize several flaky test suites

lordgamez opened a new pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959


   This PR addresses several issues in tests, addressed by each commit, so it is advised to review on a per-commit basis. Fixes included for the following test suites (more information in the commit messages):
   
   - FileUtilsTests 
     - Fix for last_write_time anomalies
   - RepoTests and PersistanceTests
     - Separate checkpoint usage when running in parallel
   - SecureSocketGetTCP tests
     - Fix timing issues
   - TailFileTests
     - Fix random generation and timing problems
   - GetTCPTests
     - Added missing ReconnectInterval property
     - Fixed recv error checking on Windows to properly try to reconnect and adjusted reconnection property values
   - Flow-Tests
     - Replace state manager provider with UnorderedMapPersistableKeyValueStoreService to avoid parallel running issues
   - DBProvenanceRepositoryTests
     - Remove an unreliable check
   - Fedora build
     - Added check for fastest mirror and retry mechanism for curl timeout problems
   - FlowControllerTests
     - Adjusted timeouts
   
   -------------------------------------------------------
   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 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] lordgamez commented on a change in pull request #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r543375546



##########
File path: docker/fedora/Dockerfile
##########
@@ -27,7 +27,11 @@ ARG MINIFI_VERSION
 ENV MINIFI_BASE_DIR /opt/minifi
 ENV MINIFI_HOME $MINIFI_BASE_DIR/nifi-minifi-cpp-$MINIFI_VERSION
 
-RUN yum -y install java-1.8.0-openjdk java-1.8.0-openjdk-devel flex bison make patch sudo git which maven libtool autoconf automake
+RUN echo "fastestmirror=True" | tee -a /etc/dnf/dnf.conf
+RUN for iter in {1..10}; do yum update -y && \
+    yum -y install java-1.8.0-openjdk java-1.8.0-openjdk-devel flex bison make patch sudo git which maven libtool autoconf automake && \
+    yum clean all && exit_code=0 && break || exit_code=$? && echo "yum error: retry $iter in 10s" && sleep 10; done; \
+    (exit $exit_code)

Review comment:
       Yes, exactly, setting the last exit code, which is then checked by the docker build.

##########
File path: docker/fedora/Dockerfile
##########
@@ -27,7 +27,11 @@ ARG MINIFI_VERSION
 ENV MINIFI_BASE_DIR /opt/minifi
 ENV MINIFI_HOME $MINIFI_BASE_DIR/nifi-minifi-cpp-$MINIFI_VERSION
 
-RUN yum -y install java-1.8.0-openjdk java-1.8.0-openjdk-devel flex bison make patch sudo git which maven libtool autoconf automake
+RUN echo "fastestmirror=True" | tee -a /etc/dnf/dnf.conf
+RUN for iter in {1..10}; do yum update -y && \
+    yum -y install java-1.8.0-openjdk java-1.8.0-openjdk-devel flex bison make patch sudo git which maven libtool autoconf automake && \
+    yum clean all && exit_code=0 && break || exit_code=$? && echo "yum error: retry $iter in 10s" && sleep 10; done; \

Review comment:
       The original issue:
   `Curl error (28): Timeout was reached for https://mirrors.fedoraproject.org/metalink?repo=fedora-29&arch=x86_64 [Connection timed out after 30000 milliseconds]`
   I'm not sure what the root cause of this, maybe something in the environment is not set up yet, so we just wait 10 seconds between tries, to wait for the environment setup to complete. The issue was quite common and disappeared after this fix. The way it is done comes from the [ElesticSearch repository](https://github.com/elastic/elasticsearch/blob/6.8/distribution/docker/src/docker/Dockerfile).




----------------------------------------------------------------
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 #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
szaszm closed pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959


   


----------------------------------------------------------------
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 #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r543362773



##########
File path: extensions/rocksdb-repos/FlowFileRepository.h
##########
@@ -61,10 +61,11 @@ class FlowFileRepository : public core::Repository, public std::enable_shared_fr
       : FlowFileRepository(name) {
   }
 
-  FlowFileRepository(const std::string repo_name = "", std::string directory = FLOWFILE_REPOSITORY_DIRECTORY, int64_t maxPartitionMillis = MAX_FLOWFILE_REPOSITORY_ENTRY_LIFE_TIME,
+  FlowFileRepository(const std::string repo_name = "", std::string checkpoint_dir = FLOWFILE_CHECKPOINT_DIRECTORY, std::string directory = FLOWFILE_REPOSITORY_DIRECTORY, int64_t maxPartitionMillis = MAX_FLOWFILE_REPOSITORY_ENTRY_LIFE_TIME,

Review comment:
       Minor, but why not const references for strings? It would remove the need for `std::move`.




----------------------------------------------------------------
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 #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r543407190



##########
File path: libminifi/test/persistence-tests/PersistenceTests.cpp
##########
@@ -326,11 +335,15 @@ TEST_CASE("Persisted flowFiles are updated on modification", "[TestP1]") {
     ff_repository->start();
     // wait for FlowFileRepository to start and notify the owners of
     // the resurrected FlowFiles
-    std::this_thread::sleep_for(std::chrono::milliseconds{100});
-
+    std::this_thread::sleep_for(std::chrono::milliseconds(100));

Review comment:
       How about waiting for a log line?




----------------------------------------------------------------
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 #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r543229726



##########
File path: libminifi/test/persistence-tests/PersistenceTests.cpp
##########
@@ -326,11 +335,15 @@ TEST_CASE("Persisted flowFiles are updated on modification", "[TestP1]") {
     ff_repository->start();
     // wait for FlowFileRepository to start and notify the owners of
     // the resurrected FlowFiles
-    std::this_thread::sleep_for(std::chrono::milliseconds{100});
-
+    std::this_thread::sleep_for(std::chrono::milliseconds(100));

Review comment:
       Is there a better way to wait for the repository to start, eg. by checking some atomic variable?  This sleep looks like it can be a source of flakiness, too.
   
   (Same comment on the two similar sleeps in RepoTests.)




----------------------------------------------------------------
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 #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r543362773



##########
File path: extensions/rocksdb-repos/FlowFileRepository.h
##########
@@ -61,10 +61,11 @@ class FlowFileRepository : public core::Repository, public std::enable_shared_fr
       : FlowFileRepository(name) {
   }
 
-  FlowFileRepository(const std::string repo_name = "", std::string directory = FLOWFILE_REPOSITORY_DIRECTORY, int64_t maxPartitionMillis = MAX_FLOWFILE_REPOSITORY_ENTRY_LIFE_TIME,
+  FlowFileRepository(const std::string repo_name = "", std::string checkpoint_dir = FLOWFILE_CHECKPOINT_DIRECTORY, std::string directory = FLOWFILE_REPOSITORY_DIRECTORY, int64_t maxPartitionMillis = MAX_FLOWFILE_REPOSITORY_ENTRY_LIFE_TIME,

Review comment:
       Minor, but why not const ref strings?

##########
File path: extensions/rocksdb-repos/FlowFileRepository.h
##########
@@ -61,10 +61,11 @@ class FlowFileRepository : public core::Repository, public std::enable_shared_fr
       : FlowFileRepository(name) {
   }
 
-  FlowFileRepository(const std::string repo_name = "", std::string directory = FLOWFILE_REPOSITORY_DIRECTORY, int64_t maxPartitionMillis = MAX_FLOWFILE_REPOSITORY_ENTRY_LIFE_TIME,
+  FlowFileRepository(const std::string repo_name = "", std::string checkpoint_dir = FLOWFILE_CHECKPOINT_DIRECTORY, std::string directory = FLOWFILE_REPOSITORY_DIRECTORY, int64_t maxPartitionMillis = MAX_FLOWFILE_REPOSITORY_ENTRY_LIFE_TIME,

Review comment:
       Minor, but why not const references for strings?




----------------------------------------------------------------
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 #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r542574442



##########
File path: libminifi/src/io/tls/TLSServerSocket.cpp
##########
@@ -87,13 +87,13 @@ void TLSServerSocket::registerCallback(std::function<bool()> accept_function, st
  * Initializes the socket
  * @return result of the creation operation.
  */
-void TLSServerSocket::registerCallback(std::function<bool()> accept_function, std::function<int(std::vector<uint8_t>*, int *)> handler) {
-  fx = [this](std::function<bool()> accept_function, std::function<int(std::vector<uint8_t>*, int *)> handler) {
+void TLSServerSocket::registerCallback(std::function<bool()> accept_function, std::function<int(std::vector<uint8_t>*, int *)> handler, std::chrono::milliseconds timeout) {
+  fx = [this](std::function<bool()> accept_function, std::function<int(std::vector<uint8_t>*, int *)> handler, std::chrono::milliseconds timeout) {
     int ret = 0;
     std::vector<int> fds;
     int size;
     while (accept_function()) {
-      int fd = select_descriptor(3000);
+      int fd = select_descriptor(timeout.count());

Review comment:
       this is a narrowing conversion from at least 45 bits (signed) to 16 bits (unsigned); it could be made safe (and warning-free) with `gsl::narrow`




----------------------------------------------------------------
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] lordgamez commented on a change in pull request #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r543433800



##########
File path: docker/fedora/Dockerfile
##########
@@ -27,7 +27,11 @@ ARG MINIFI_VERSION
 ENV MINIFI_BASE_DIR /opt/minifi
 ENV MINIFI_HOME $MINIFI_BASE_DIR/nifi-minifi-cpp-$MINIFI_VERSION
 
-RUN yum -y install java-1.8.0-openjdk java-1.8.0-openjdk-devel flex bison make patch sudo git which maven libtool autoconf automake
+RUN echo "fastestmirror=True" | tee -a /etc/dnf/dnf.conf
+RUN for iter in {1..10}; do yum update -y && \
+    yum -y install java-1.8.0-openjdk java-1.8.0-openjdk-devel flex bison make patch sudo git which maven libtool autoconf automake && \
+    yum clean all && exit_code=0 && break || exit_code=$? && echo "yum error: retry $iter in 10s" && sleep 10; done; \
+    (exit $exit_code)

Review comment:
       The command above is run as a shell command in the docker build container, and the `return` keyword could only be used in the scope of a shell function.




----------------------------------------------------------------
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] lordgamez commented on a change in pull request #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r542602723



##########
File path: libminifi/src/io/tls/TLSServerSocket.cpp
##########
@@ -87,13 +87,13 @@ void TLSServerSocket::registerCallback(std::function<bool()> accept_function, st
  * Initializes the socket
  * @return result of the creation operation.
  */
-void TLSServerSocket::registerCallback(std::function<bool()> accept_function, std::function<int(std::vector<uint8_t>*, int *)> handler) {
-  fx = [this](std::function<bool()> accept_function, std::function<int(std::vector<uint8_t>*, int *)> handler) {
+void TLSServerSocket::registerCallback(std::function<bool()> accept_function, std::function<int(std::vector<uint8_t>*, int *)> handler, std::chrono::milliseconds timeout) {
+  fx = [this](std::function<bool()> accept_function, std::function<int(std::vector<uint8_t>*, int *)> handler, std::chrono::milliseconds timeout) {
     int ret = 0;
     std::vector<int> fds;
     int size;
     while (accept_function()) {
-      int fd = select_descriptor(3000);
+      int fd = select_descriptor(timeout.count());

Review comment:
       Fixed in [bfc8394](https://github.com/apache/nifi-minifi-cpp/pull/959/commits/bfc83949f4c3549d5a85fc0250068eea7e7801d1)




----------------------------------------------------------------
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] lordgamez commented on a change in pull request #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r544183525



##########
File path: libminifi/test/persistence-tests/PersistenceTests.cpp
##########
@@ -326,11 +335,15 @@ TEST_CASE("Persisted flowFiles are updated on modification", "[TestP1]") {
     ff_repository->start();
     // wait for FlowFileRepository to start and notify the owners of
     // the resurrected FlowFiles
-    std::this_thread::sleep_for(std::chrono::milliseconds{100});
-
+    std::this_thread::sleep_for(std::chrono::milliseconds(100));

Review comment:
       Added log checks instead of sleeps in the PersistenceTests in [eaf4925](https://github.com/apache/nifi-minifi-cpp/pull/959/commits/eaf49254954dcbc12125b1d9d14aa7be829b7d97). Checking for "Found connection for" log entry which indicates a the restore procedure to be taken place seems to be working for our case.




----------------------------------------------------------------
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 #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r542496017



##########
File path: libminifi/test/persistence-tests/PersistenceTests.cpp
##########
@@ -326,10 +334,12 @@ TEST_CASE("Persisted flowFiles are updated on modification", "[TestP1]") {
     ff_repository->start();
     // wait for FlowFileRepository to start and notify the owners of
     // the resurrected FlowFiles
-    std::this_thread::sleep_for(std::chrono::milliseconds{100});
-
     std::set<std::shared_ptr<core::FlowFile>> expired;
-    auto file = flow.output->poll(expired);
+    std::shared_ptr<org::apache::nifi::minifi::core::FlowFile> file = nullptr;
+    for (auto i = 0; i < 20 && !file; ++i) {

Review comment:
       you could use `verifyEventHappenedInPollTime` (in IntegrationTestUtils.h) for this; also in two other cases in RepoTests

##########
File path: libminifi/include/io/tls/TLSServerSocket.h
##########
@@ -53,7 +53,7 @@ class TLSServerSocket : public BaseServerSocket, public TLSSocket {
   /**
    * Registers a call back and starts the read for the server socket.
    */
-  void registerCallback(std::function<bool()> accept_function, std::function<int(std::vector<uint8_t>*, int *)> handler);
+  void registerCallback(std::function<bool()> accept_function, std::function<int(std::vector<uint8_t>*, int *)> handler, uint16_t timeout = 3000);

Review comment:
       using `std::chrono::milliseconds` would be clearer than `uint16_t`




----------------------------------------------------------------
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 #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r543358865



##########
File path: docker/fedora/Dockerfile
##########
@@ -27,7 +27,11 @@ ARG MINIFI_VERSION
 ENV MINIFI_BASE_DIR /opt/minifi
 ENV MINIFI_HOME $MINIFI_BASE_DIR/nifi-minifi-cpp-$MINIFI_VERSION
 
-RUN yum -y install java-1.8.0-openjdk java-1.8.0-openjdk-devel flex bison make patch sudo git which maven libtool autoconf automake
+RUN echo "fastestmirror=True" | tee -a /etc/dnf/dnf.conf
+RUN for iter in {1..10}; do yum update -y && \
+    yum -y install java-1.8.0-openjdk java-1.8.0-openjdk-devel flex bison make patch sudo git which maven libtool autoconf automake && \
+    yum clean all && exit_code=0 && break || exit_code=$? && echo "yum error: retry $iter in 10s" && sleep 10; done; \
+    (exit $exit_code)

Review comment:
       What does this do? It opens a subshell and exits it with exit code?




----------------------------------------------------------------
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] lordgamez commented on a change in pull request #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r543237706



##########
File path: libminifi/test/persistence-tests/PersistenceTests.cpp
##########
@@ -326,11 +335,15 @@ TEST_CASE("Persisted flowFiles are updated on modification", "[TestP1]") {
     ff_repository->start();
     // wait for FlowFileRepository to start and notify the owners of
     // the resurrected FlowFiles
-    std::this_thread::sleep_for(std::chrono::milliseconds{100});
-
+    std::this_thread::sleep_for(std::chrono::milliseconds(100));

Review comment:
       Unfortunately I could not find a way to check it, as the ff repository only sets the `running` variable to true and goes on to start a separate thread which watches this `running` variable.




----------------------------------------------------------------
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] lordgamez commented on a change in pull request #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r543430274



##########
File path: libminifi/test/rocksdb-tests/DBProvenanceRepositoryTests.cpp
##########
@@ -103,8 +103,6 @@ TEST_CASE("Test time limit", "[timeLimitTest]") {
 
   provisionRepo(provdb, keyCount / 2, 102400);
 
-  REQUIRE(provdb.getKeyCount() == 250);
-
   /**

Review comment:
       I think the comment is okay as we do put 250 entries to the DB. The problem here was that sometimes when we get to this point of the execution the key count is already 96 instead of 250, as the serialized records have probably already expired.




----------------------------------------------------------------
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] lordgamez commented on a change in pull request #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r543193837



##########
File path: libminifi/test/persistence-tests/PersistenceTests.cpp
##########
@@ -326,10 +334,12 @@ TEST_CASE("Persisted flowFiles are updated on modification", "[TestP1]") {
     ff_repository->start();
     // wait for FlowFileRepository to start and notify the owners of
     // the resurrected FlowFiles
-    std::this_thread::sleep_for(std::chrono::milliseconds{100});
-
     std::set<std::shared_ptr<core::FlowFile>> expired;
-    auto file = flow.output->poll(expired);
+    std::shared_ptr<org::apache::nifi::minifi::core::FlowFile> file = nullptr;
+    for (auto i = 0; i < 20 && !file; ++i) {

Review comment:
       Additional fix in [7118255](https://github.com/apache/nifi-minifi-cpp/pull/959/commits/71182553d44298eb0e2f400425d0133995410b45) as we still need an additional wait for the flowfile repository to start in the test.




----------------------------------------------------------------
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 #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r543358223



##########
File path: docker/fedora/Dockerfile
##########
@@ -27,7 +27,11 @@ ARG MINIFI_VERSION
 ENV MINIFI_BASE_DIR /opt/minifi
 ENV MINIFI_HOME $MINIFI_BASE_DIR/nifi-minifi-cpp-$MINIFI_VERSION
 
-RUN yum -y install java-1.8.0-openjdk java-1.8.0-openjdk-devel flex bison make patch sudo git which maven libtool autoconf automake
+RUN echo "fastestmirror=True" | tee -a /etc/dnf/dnf.conf
+RUN for iter in {1..10}; do yum update -y && \
+    yum -y install java-1.8.0-openjdk java-1.8.0-openjdk-devel flex bison make patch sudo git which maven libtool autoconf automake && \
+    yum clean all && exit_code=0 && break || exit_code=$? && echo "yum error: retry $iter in 10s" && sleep 10; done; \

Review comment:
       What was the issue here? Why do we have to sleep?




----------------------------------------------------------------
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 #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r543410257



##########
File path: libminifi/test/rocksdb-tests/DBProvenanceRepositoryTests.cpp
##########
@@ -103,8 +103,6 @@ TEST_CASE("Test time limit", "[timeLimitTest]") {
 
   provisionRepo(provdb, keyCount / 2, 102400);
 
-  REQUIRE(provdb.getKeyCount() == 250);
-
   /**

Review comment:
       What happened here? Can you modify the comment as well why we dont have 250 entries?




----------------------------------------------------------------
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] lordgamez commented on a change in pull request #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r543427100



##########
File path: libminifi/test/persistence-tests/PersistenceTests.cpp
##########
@@ -326,11 +335,15 @@ TEST_CASE("Persisted flowFiles are updated on modification", "[TestP1]") {
     ff_repository->start();
     // wait for FlowFileRepository to start and notify the owners of
     // the resurrected FlowFiles
-    std::this_thread::sleep_for(std::chrono::milliseconds{100});
-
+    std::this_thread::sleep_for(std::chrono::milliseconds(100));

Review comment:
       As I checked the RepoTests there isn't anything logged there from the FlowFileRepository perspective:
   ```
   [2020-12-15 15:59:02.639] [org::apache::nifi::minifi::core::logging::LoggerConfiguration] [debug] org::apache::nifi::minifi::FlowFileRecord logger got sinks from namespace root and level error from namespace root
   [2020-12-15 15:59:02.639] [org::apache::nifi::minifi::core::logging::LoggerConfiguration] [debug] org::apache::nifi::minifi::utils::IdGenerator logger got sinks from namespace root and level error from namespace root
   [2020-12-15 15:59:02.639] [org::apache::nifi::minifi::core::logging::LoggerConfiguration] [debug] org::apache::nifi::minifi::core::FlowFile logger got sinks from namespace root and level error from namespace root
   [2020-12-15 15:59:02.639] [org::apache::nifi::minifi::core::logging::LoggerConfiguration] [debug] org::apache::nifi::minifi::provenance::ProvenanceEventRecord logger got sinks from namespace root and level error from namespace root
   [2020-12-15 15:59:02.639] [org::apache::nifi::minifi::core::logging::LoggerConfiguration] [debug] org::apache::nifi::minifi::internal::RocksDatabase logger got sinks from namespace root and level error from namespace root
   [2020-12-15 15:59:02.639] [org::apache::nifi::minifi::core::logging::LoggerConfiguration] [debug] org::apache::nifi::minifi::Properties logger got sinks from namespace root and level error from namespace root
   [2020-12-15 15:59:02.639] [org::apache::nifi::minifi::core::logging::LoggerConfiguration] [debug] Set following pattern on loggers: [%Y-%m-%d %H:%M:%S.%e] [%n] [%l] %v
   [2020-12-15 15:59:02.639] [org::apache::nifi::minifi::core::logging::LoggerConfiguration] [debug] LogTestController logger got sinks from namespace root and level info from namespace LogTestController
   [2020-12-15 15:59:02.639] [org::apache::nifi::minifi::core::logging::LoggerConfiguration] [debug] org::apache::nifi::minifi::core::Connectable logger got sinks from namespace root and level error from namespace root
   [2020-12-15 15:59:02.639] [org::apache::nifi::minifi::core::logging::LoggerConfiguration] [debug] org::apache::nifi::minifi::core::Repository logger got sinks from namespace root and level error from namespace root
   [2020-12-15 15:59:02.639] [org::apache::nifi::minifi::core::logging::LoggerConfiguration] [debug] org::apache::nifi::minifi::core::repository::VolatileRepository<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > logger got sinks from namespace root and level error from namespace root
   [2020-12-15 15:59:02.639] [org::apache::nifi::minifi::core::logging::LoggerConfiguration] [debug] org::apache::nifi::minifi::core::repository::VolatileContentRepository logger got sinks from namespace root and level error from namespace root
   [2020-12-15 15:59:02.639] [org::apache::nifi::minifi::core::logging::LoggerConfiguration] [debug] org::apache::nifi::minifi::Connection logger got sinks from namespace root and level error from namespace root
   [2020-12-15 15:59:02.639] [org::apache::nifi::minifi::core::logging::LoggerConfiguration] [debug] org::apache::nifi::minifi::core::repository::FlowFileRepository logger got sinks from namespace root and level error from namespace root
   ```




----------------------------------------------------------------
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] lordgamez commented on a change in pull request #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r542554296



##########
File path: libminifi/test/persistence-tests/PersistenceTests.cpp
##########
@@ -326,10 +334,12 @@ TEST_CASE("Persisted flowFiles are updated on modification", "[TestP1]") {
     ff_repository->start();
     // wait for FlowFileRepository to start and notify the owners of
     // the resurrected FlowFiles
-    std::this_thread::sleep_for(std::chrono::milliseconds{100});
-
     std::set<std::shared_ptr<core::FlowFile>> expired;
-    auto file = flow.output->poll(expired);
+    std::shared_ptr<org::apache::nifi::minifi::core::FlowFile> file = nullptr;
+    for (auto i = 0; i < 20 && !file; ++i) {

Review comment:
       Fixed in [a20314b](https://github.com/apache/nifi-minifi-cpp/pull/959/commits/a20314b080d4fded9d71d61d68d6be616032eb4f)




----------------------------------------------------------------
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 #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r543407190



##########
File path: libminifi/test/persistence-tests/PersistenceTests.cpp
##########
@@ -326,11 +335,15 @@ TEST_CASE("Persisted flowFiles are updated on modification", "[TestP1]") {
     ff_repository->start();
     // wait for FlowFileRepository to start and notify the owners of
     // the resurrected FlowFiles
-    std::this_thread::sleep_for(std::chrono::milliseconds{100});
-
+    std::this_thread::sleep_for(std::chrono::milliseconds(100));

Review comment:
       How about waiting for a log line? Is there a candidate?




----------------------------------------------------------------
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] lordgamez commented on a change in pull request #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r543454567



##########
File path: extensions/standard-processors/processors/GetTCP.cpp
##########
@@ -148,7 +149,13 @@ void GetTCP::onSchedule(const std::shared_ptr<core::ProcessContext> &context, co
 
   logger_->log_trace("EOM is defined as %i", endOfMessageByte);
 
-  context->getProperty(ReconnectInterval.getName(), reconnect_interval_);
+  std::string reconnect_interval_str;
+  if (context->getProperty(ReconnectInterval.getName(), reconnect_interval_str) &&
+      core::Property::getTimeMSFromString(reconnect_interval_str, reconnect_interval_)) {
+    logger_->log_debug("Reconnect interval is %d ms", reconnect_interval_);

Review comment:
       Done in [c0110dc](https://github.com/apache/nifi-minifi-cpp/pull/959/commits/c0110dca0937ac4af93e32d7ccab8347132183c7)

##########
File path: extensions/rocksdb-repos/FlowFileRepository.h
##########
@@ -61,10 +61,11 @@ class FlowFileRepository : public core::Repository, public std::enable_shared_fr
       : FlowFileRepository(name) {
   }
 
-  FlowFileRepository(const std::string repo_name = "", std::string directory = FLOWFILE_REPOSITORY_DIRECTORY, int64_t maxPartitionMillis = MAX_FLOWFILE_REPOSITORY_ENTRY_LIFE_TIME,
+  FlowFileRepository(const std::string repo_name = "", std::string checkpoint_dir = FLOWFILE_CHECKPOINT_DIRECTORY, std::string directory = FLOWFILE_REPOSITORY_DIRECTORY, int64_t maxPartitionMillis = MAX_FLOWFILE_REPOSITORY_ENTRY_LIFE_TIME,

Review comment:
       Done in [c0110dc](https://github.com/apache/nifi-minifi-cpp/pull/959/commits/c0110dca0937ac4af93e32d7ccab8347132183c7)




----------------------------------------------------------------
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] lordgamez commented on a change in pull request #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r544183525



##########
File path: libminifi/test/persistence-tests/PersistenceTests.cpp
##########
@@ -326,11 +335,15 @@ TEST_CASE("Persisted flowFiles are updated on modification", "[TestP1]") {
     ff_repository->start();
     // wait for FlowFileRepository to start and notify the owners of
     // the resurrected FlowFiles
-    std::this_thread::sleep_for(std::chrono::milliseconds{100});
-
+    std::this_thread::sleep_for(std::chrono::milliseconds(100));

Review comment:
       Added log checks instead of sleeps in the PersistenceTests in [eaf4925](https://github.com/apache/nifi-minifi-cpp/pull/959/commits/eaf49254954dcbc12125b1d9d14aa7be829b7d97). Checking for "Found connection for" log entry which indicates a the restore procedure to be taken place seems to be working for our case. Sleeping in RepoTests was also unnecessary, so it is 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] lordgamez commented on a change in pull request #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r544237854



##########
File path: libminifi/test/persistence-tests/PersistenceTests.cpp
##########
@@ -326,11 +335,15 @@ TEST_CASE("Persisted flowFiles are updated on modification", "[TestP1]") {
     ff_repository->start();
     // wait for FlowFileRepository to start and notify the owners of
     // the resurrected FlowFiles
-    std::this_thread::sleep_for(std::chrono::milliseconds{100});
-
+    std::this_thread::sleep_for(std::chrono::milliseconds(100));

Review comment:
       It seems unfortunately wait is still needed in RepoTests and one of them cannot be checked by any log entries either, so one sleep is necessary (updated in [986ff8d](https://github.com/apache/nifi-minifi-cpp/pull/959/commits/986ff8db637d6551ff5e0d68f02656b6039a28af)).




----------------------------------------------------------------
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 #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r542574442



##########
File path: libminifi/src/io/tls/TLSServerSocket.cpp
##########
@@ -87,13 +87,13 @@ void TLSServerSocket::registerCallback(std::function<bool()> accept_function, st
  * Initializes the socket
  * @return result of the creation operation.
  */
-void TLSServerSocket::registerCallback(std::function<bool()> accept_function, std::function<int(std::vector<uint8_t>*, int *)> handler) {
-  fx = [this](std::function<bool()> accept_function, std::function<int(std::vector<uint8_t>*, int *)> handler) {
+void TLSServerSocket::registerCallback(std::function<bool()> accept_function, std::function<int(std::vector<uint8_t>*, int *)> handler, std::chrono::milliseconds timeout) {
+  fx = [this](std::function<bool()> accept_function, std::function<int(std::vector<uint8_t>*, int *)> handler, std::chrono::milliseconds timeout) {
     int ret = 0;
     std::vector<int> fds;
     int size;
     while (accept_function()) {
-      int fd = select_descriptor(3000);
+      int fd = select_descriptor(timeout.count());

Review comment:
       this is a narrowing conversion from at least 45 bits to 16 bits; could be made safe (and warning-free) with `gsl::narrow`




----------------------------------------------------------------
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 #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r543388359



##########
File path: extensions/standard-processors/processors/GetTCP.cpp
##########
@@ -148,7 +149,13 @@ void GetTCP::onSchedule(const std::shared_ptr<core::ProcessContext> &context, co
 
   logger_->log_trace("EOM is defined as %i", endOfMessageByte);
 
-  context->getProperty(ReconnectInterval.getName(), reconnect_interval_);
+  std::string reconnect_interval_str;
+  if (context->getProperty(ReconnectInterval.getName(), reconnect_interval_str) &&
+      core::Property::getTimeMSFromString(reconnect_interval_str, reconnect_interval_)) {
+    logger_->log_debug("Reconnect interval is %d ms", reconnect_interval_);

Review comment:
       Minor but this might overflow for `uint64_t`:
   https://godbolt.org/z/WsTK8W




----------------------------------------------------------------
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] lordgamez commented on a change in pull request #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r542553948



##########
File path: libminifi/include/io/tls/TLSServerSocket.h
##########
@@ -53,7 +53,7 @@ class TLSServerSocket : public BaseServerSocket, public TLSSocket {
   /**
    * Registers a call back and starts the read for the server socket.
    */
-  void registerCallback(std::function<bool()> accept_function, std::function<int(std::vector<uint8_t>*, int *)> handler);
+  void registerCallback(std::function<bool()> accept_function, std::function<int(std::vector<uint8_t>*, int *)> handler, uint16_t timeout = 3000);

Review comment:
       Fixed in [e35f799](https://github.com/apache/nifi-minifi-cpp/pull/959/commits/e35f799294095be494056f415a24b7350192e61d)




----------------------------------------------------------------
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 #959: MINIFICPP-1424 Stabilize several flaky test suites

Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #959:
URL: https://github.com/apache/nifi-minifi-cpp/pull/959#discussion_r543418596



##########
File path: docker/fedora/Dockerfile
##########
@@ -27,7 +27,11 @@ ARG MINIFI_VERSION
 ENV MINIFI_BASE_DIR /opt/minifi
 ENV MINIFI_HOME $MINIFI_BASE_DIR/nifi-minifi-cpp-$MINIFI_VERSION
 
-RUN yum -y install java-1.8.0-openjdk java-1.8.0-openjdk-devel flex bison make patch sudo git which maven libtool autoconf automake
+RUN echo "fastestmirror=True" | tee -a /etc/dnf/dnf.conf
+RUN for iter in {1..10}; do yum update -y && \
+    yum -y install java-1.8.0-openjdk java-1.8.0-openjdk-devel flex bison make patch sudo git which maven libtool autoconf automake && \
+    yum clean all && exit_code=0 && break || exit_code=$? && echo "yum error: retry $iter in 10s" && sleep 10; done; \
+    (exit $exit_code)

Review comment:
       Why not just `return $exit_code`?




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