You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/01/11 13:05:01 UTC

[GitHub] [nifi-minifi-cpp] lordgamez opened a new pull request #974: MINIFICPP-1449 Add pause and resume command to C2

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


   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 #974: MINIFICPP-1449 Add pause and resume command to C2

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



##########
File path: libminifi/include/core/state/UpdateController.h
##########
@@ -148,7 +148,16 @@ class UpdateRunner : public utils::AfterExecute<Update> {
   std::chrono::milliseconds delay_;
 };
 
-class StateController {
+class PausableStateController {
+ public:
+  virtual ~PausableStateController() = default;
+
+  virtual int16_t pause() = 0;
+
+  virtual int16_t resume() = 0;
+};

Review comment:
       I updated the name of the interface in [11cca31](https://github.com/apache/nifi-minifi-cpp/pull/974/commits/11cca310b1dd516fbdaf0e1264fbe6f0c552803e). I would stay with this implementation now instead of a Pausable concept, it seems more simple and explicit for me.




----------------------------------------------------------------
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 #974: MINIFICPP-1449 Add pause and resume command to C2

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



##########
File path: libminifi/include/core/state/UpdateController.h
##########
@@ -148,7 +148,16 @@ class UpdateRunner : public utils::AfterExecute<Update> {
   std::chrono::milliseconds delay_;
 };
 
-class StateController {
+class PausableStateController {
+ public:
+  virtual ~PausableStateController() = default;
+
+  virtual int16_t pause() = 0;
+
+  virtual int16_t resume() = 0;
+};

Review comment:
       There is nothing in this interface that is related to `StateController`. This interface should probably be named `Pausable` instead. This also implies some C2Agent and related changes. The C2 part probably doesn't need to care what kind of object it pauses, but just pause and resume something that is `Pausable` and was passed as the ctor argument to be paused/resumed.
   
   I was thinking about defining `Pausable` as a runtime concept with reference semantics, but that is only if you like the idea of not having to depend on/derive from a class to "be" `Pausable` enough to take on significantly increased implementation complexity. The implementation would be a wrapper class with a pointer to a private abstract class (concept) similar to the current one and a constructor template that instantiates a private class template (model) implementing the private concept class.




----------------------------------------------------------------
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 pull request #974: MINIFICPP-1449 Add pause and resume command to C2

Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on pull request #974:
URL: https://github.com/apache/nifi-minifi-cpp/pull/974#issuecomment-759273848


   > > Wonder if tasks need to have the property to indicate paused state.
   > > So currently I think we have a dedicated threadpool for c2 (heartbeating). Because of this the flowcontroller's threadpool can be completely paused.
   > > In this case the scheduling agent and the tasks don't need to be impacted, only the threadpool workers top picking up tasks while the flowcontroller is paused.
   > > What do you think?
   > 
   > That's actually what I wanted to go with first, but a problem occurred when I implemented it. When the ThreadPool dequeues the worker queue and sees that the dequeued task is in stopped state it does not re-enqueue the task, but simply throws it away. The task remains in the task states map, but changing its state has no effect afterwards. This is why I wanted to distinguish a stopped task from a paused task by moving the paused tasks to separate map when they are processed and later re-enqueueing them when they are resumed thus keeping the original behavior of the stopped tasks.
   
   I feel like there are two contending "pause/resume" operation semantics, one regarding the FlowController (@arpadboda ), and one regarding the processors (@lordgamez)
   
   1. which one is the current PR about? (it seem to me, that the first one)
   2. if the first one, do we plan on extending it to support the second one?
   3. if the first one, and we don't plan on supporting processor-level pause/resume why doesn't a ThreadPool simply stop dequeuing tasks on pause


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] arpadboda commented on pull request #974: MINIFICPP-1449 Add pause and resume command to C2

Posted by GitBox <gi...@apache.org>.
arpadboda commented on pull request #974:
URL: https://github.com/apache/nifi-minifi-cpp/pull/974#issuecomment-759327316


   @adamdebreceni : currently it's about the first (stopping the whole flow). Being able to stop one processor might be handy in the future. 
   
   @lordgamez : for pausing the whole threadpool, I think the easy solution is sending back the worker threads to sleep when they wake up. 
   Somewhere in ```ThreadPool<T>::run_tasks```
   
   I just realised that stopping the worker_queue_ actually results in this. 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #974: MINIFICPP-1449 Add pause and resume command to C2

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



##########
File path: libminifi/src/FlowController.cpp
##########
@@ -360,6 +360,33 @@ int16_t FlowController::start() {
   }
 }
 
+int16_t FlowController::pause() {
+  std::lock_guard<std::recursive_mutex> flow_lock(mutex_);
+  if (!running_) {
+    logger_->log_warn("Can not pause flow controller that is not running");
+    return 0;
+  }
+
+  logger_->log_info("Pausing Flow Controller");
+  timer_scheduler_->pause();

Review comment:
       As far as I remember the threadpool is currently the member of flowcontroller. 
   Only a reference to that is passed to all schedulers, so schedulers share a common threadpool. 
   
   Which means that we notify all scheduling agents to indirectly pause our own threadpool, but at least it's done 3 times. :)
   
   I don't think that the scheduling agents even need to be aware to being paused and resumed. I doens't hurt, but I think pausing the threadpool should be the responsibility of the flowcontroller, which makes this code less complex, too. 
   




----------------------------------------------------------------
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 pull request #974: MINIFICPP-1449 Add pause and resume command to C2

Posted by GitBox <gi...@apache.org>.
lordgamez commented on pull request #974:
URL: https://github.com/apache/nifi-minifi-cpp/pull/974#issuecomment-759323794


   > > > Wonder if tasks need to have the property to indicate paused state.
   > > > So currently I think we have a dedicated threadpool for c2 (heartbeating). Because of this the flowcontroller's threadpool can be completely paused.
   > > > In this case the scheduling agent and the tasks don't need to be impacted, only the threadpool workers top picking up tasks while the flowcontroller is paused.
   > > > What do you think?
   > > 
   > > 
   > > That's actually what I wanted to go with first, but a problem occurred when I implemented it. When the ThreadPool dequeues the worker queue and sees that the dequeued task is in stopped state it does not re-enqueue the task, but simply throws it away. The task remains in the task states map, but changing its state has no effect afterwards. This is why I wanted to distinguish a stopped task from a paused task by moving the paused tasks to separate map when they are processed and later re-enqueueing them when they are resumed thus keeping the original behavior of the stopped tasks.
   > 
   > I feel like there are two contending "pause/resume" operation semantics, one regarding the FlowController (@arpadboda ), and one regarding the processors (@lordgamez)
   > 
   > 1. which one is the current PR about? (it seem to me, that the first one)
   > 2. if the first one, do we plan on extending it to support the second one?
   > 3. if the first one, and we don't plan on supporting processor-level pause/resume why doesn't a ThreadPool simply stop dequeuing tasks on pause
   
   I suppose in our case as we only want to pause the workflow, it would suffice to stop the dequeuing the the tasks. I am not sure if we have any plans in the future to pause-resume on processor level, or if we should have that option open, maybe @arpadboda can answer that.


----------------------------------------------------------------
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 #974: MINIFICPP-1449 Add pause and resume command to C2

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



##########
File path: libminifi/src/FlowController.cpp
##########
@@ -360,6 +360,33 @@ int16_t FlowController::start() {
   }
 }
 
+int16_t FlowController::pause() {
+  std::lock_guard<std::recursive_mutex> flow_lock(mutex_);
+  if (!running_) {
+    logger_->log_warn("Can not pause flow controller that is not running");
+    return 0;
+  }
+
+  logger_->log_info("Pausing Flow Controller");
+  timer_scheduler_->pause();

Review comment:
       Thanks, I totally missed that they shared the same threadpool! I fixed it in [e69f7cf](https://github.com/apache/nifi-minifi-cpp/pull/974/commits/e69f7cfdcff8ec6fa3aa3c4e34199b90e9761270)




----------------------------------------------------------------
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 pull request #974: MINIFICPP-1449 Add pause and resume command to C2

Posted by GitBox <gi...@apache.org>.
lordgamez commented on pull request #974:
URL: https://github.com/apache/nifi-minifi-cpp/pull/974#issuecomment-766708979






----------------------------------------------------------------
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 #974: MINIFICPP-1449 Add pause and resume command to C2

Posted by GitBox <gi...@apache.org>.
szaszm commented on pull request #974:
URL: https://github.com/apache/nifi-minifi-cpp/pull/974#issuecomment-766801370


   Please don't forget to update the [C2 Design](https://cwiki.apache.org/confluence/display/MINIFI/C2+Design) wiki page.


----------------------------------------------------------------
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 #974: MINIFICPP-1449 Add pause and resume command to C2

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



##########
File path: libminifi/include/core/state/UpdateController.h
##########
@@ -148,7 +148,16 @@ class UpdateRunner : public utils::AfterExecute<Update> {
   std::chrono::milliseconds delay_;
 };
 
-class StateController {
+class PausableStateController {
+ public:
+  virtual ~PausableStateController() = default;
+
+  virtual int16_t pause() = 0;
+
+  virtual int16_t resume() = 0;
+};

Review comment:
       There is nothing in this interface that is related to `StateController`. This interface should probably be named `Pausable` instead. This also implies some C2Agent and related changes. The C2 part probably doesn't need to care what kind of object it pauses, but just pause and resume something that is `Pausable` and was passed as the ctor argument to be paused/resumed.
   
   I was thinking about defining `Pausable` as a runtime concept with reference semantics, but that is only if you like the idea of not having to depend on/derive from a class to "be" `Pausable` enough to take on significantly increased implementation complexity. The implementation would be a wrapper class with a pointer to a private abstract class (concept) similar to the current one and a constructor template that instantiates a private class template (model) implementing the private concept class.




----------------------------------------------------------------
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 #974: MINIFICPP-1449 Add pause and resume command to C2

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



##########
File path: libminifi/include/core/state/UpdateController.h
##########
@@ -148,7 +148,16 @@ class UpdateRunner : public utils::AfterExecute<Update> {
   std::chrono::milliseconds delay_;
 };
 
-class StateController {
+class PausableStateController {
+ public:
+  virtual ~PausableStateController() = default;
+
+  virtual int16_t pause() = 0;
+
+  virtual int16_t resume() = 0;
+};

Review comment:
       I updated the name of the interface in [11cca31](https://github.com/apache/nifi-minifi-cpp/pull/974/commits/11cca310b1dd516fbdaf0e1264fbe6f0c552803e). I would stay with this implementation now instead of a Pausable concept, it seems more simple and explicit for me.




----------------------------------------------------------------
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 #974: MINIFICPP-1449 Add pause and resume command to C2

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


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] arpadboda commented on pull request #974: MINIFICPP-1449 Add pause and resume command to C2

Posted by GitBox <gi...@apache.org>.
arpadboda commented on pull request #974:
URL: https://github.com/apache/nifi-minifi-cpp/pull/974#issuecomment-764799268


   I think we should investigate the CI failure here:
   
   ```
   C2PauseResumeTest: /home/runner/work/nifi-minifi-cpp/nifi-minifi-cpp/extensions/http-curl/tests/HTTPHandlers.h:432: void HeartbeatHandler::verify(mg_connection*): Assertion `ok' failed.
   ```


----------------------------------------------------------------
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 #974: MINIFICPP-1449 Add pause and resume command to C2

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



##########
File path: libminifi/include/utils/ThreadPool.h
##########
@@ -344,11 +364,13 @@ class ThreadPool {
 // notification for new delayed tasks that's before the current ones
   std::condition_variable delayed_task_available_;
 // map to identify if a task should be
-  std::map<TaskId, bool> task_status_;
+  std::map<TaskId, TaskState> task_status_;
 // manager mutex
   std::recursive_mutex manager_mutex_;
   // thread pool name
   std::string name_;
+  // map of paused tasks moved from worker queue that should not run at the time
+  std::map<TaskId, Worker<T>> paused_tasks_;

Review comment:
       Thanks for the info, I was not aware of that. I modified the map to be able to hold multiple tasks for a taskid in [cd5817c](https://github.com/apache/nifi-minifi-cpp/pull/974/commits/cd5817cd7254305b382734d954ebc5b659453519)




----------------------------------------------------------------
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 edited a comment on pull request #974: MINIFICPP-1449 Add pause and resume command to C2

Posted by GitBox <gi...@apache.org>.
lordgamez edited a comment on pull request #974:
URL: https://github.com/apache/nifi-minifi-cpp/pull/974#issuecomment-759493161


   @adamdebreceni @arpadboda I changed the semantics to stop the thread pool when we pause and restart again when we resume in [0c57323](https://github.com/apache/nifi-minifi-cpp/pull/974/commits/0c57323c28b727854786651e440617d3133cc505). I misunderstood a previous comment as previously I stopped the ThreadedScheduler not the ThreadPool. This time it works by only stopping the ThreadPool.


----------------------------------------------------------------
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 edited a comment on pull request #974: MINIFICPP-1449 Add pause and resume command to C2

Posted by GitBox <gi...@apache.org>.
lordgamez edited a comment on pull request #974:
URL: https://github.com/apache/nifi-minifi-cpp/pull/974#issuecomment-759323794


   > > > Wonder if tasks need to have the property to indicate paused state.
   > > > So currently I think we have a dedicated threadpool for c2 (heartbeating). Because of this the flowcontroller's threadpool can be completely paused.
   > > > In this case the scheduling agent and the tasks don't need to be impacted, only the threadpool workers top picking up tasks while the flowcontroller is paused.
   > > > What do you think?
   > > 
   > > 
   > > That's actually what I wanted to go with first, but a problem occurred when I implemented it. When the ThreadPool dequeues the worker queue and sees that the dequeued task is in stopped state it does not re-enqueue the task, but simply throws it away. The task remains in the task states map, but changing its state has no effect afterwards. This is why I wanted to distinguish a stopped task from a paused task by moving the paused tasks to separate map when they are processed and later re-enqueueing them when they are resumed thus keeping the original behavior of the stopped tasks.
   > 
   > I feel like there are two contending "pause/resume" operation semantics, one regarding the FlowController (@arpadboda ), and one regarding the processors (@lordgamez)
   > 
   > 1. which one is the current PR about? (it seem to me, that the first one)
   > 2. if the first one, do we plan on extending it to support the second one?
   > 3. if the first one, and we don't plan on supporting processor-level pause/resume why doesn't a ThreadPool simply stop dequeuing tasks on pause
   
   I suppose in our case as we only want to pause the workflow, it would suffice to stop the dequeuing of the tasks. I am not sure if we have any plans in the future to pause-resume on processor level, or if we should have that option open, maybe @arpadboda can answer that.


----------------------------------------------------------------
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 pull request #974: MINIFICPP-1449 Add pause and resume command to C2

Posted by GitBox <gi...@apache.org>.
lordgamez commented on pull request #974:
URL: https://github.com/apache/nifi-minifi-cpp/pull/974#issuecomment-758743776


   > Wonder if tasks need to have the property to indicate paused state.
   > 
   > So currently I think we have a dedicated threadpool for c2 (heartbeating). Because of this the flowcontroller's threadpool can be completely paused.
   > 
   > In this case the scheduling agent and the tasks don't need to be impacted, only the threadpool workers top picking up tasks while the flowcontroller is paused.
   > 
   > What do you think?
   
   That's actually what I wanted to go with first, but a problem occurred when I implemented it. When the ThreadPool dequeues the worker queue and sees that the dequeued task is in stopped state it does not re-enqueue the task, but simply throws it away. The task remains in the task states map, but changing its state has no effect afterwards. This is why I wanted to distinguish a stopped task from a paused task by moving the paused tasks to separate map when they are processed and later re-enqueueing them when they are resumed thus keeping the original behavior of the stopped tasks.


----------------------------------------------------------------
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 #974: MINIFICPP-1449 Add pause and resume command to C2

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



##########
File path: libminifi/include/utils/ThreadPool.h
##########
@@ -344,11 +364,13 @@ class ThreadPool {
 // notification for new delayed tasks that's before the current ones
   std::condition_variable delayed_task_available_;
 // map to identify if a task should be
-  std::map<TaskId, bool> task_status_;
+  std::map<TaskId, TaskState> task_status_;
 // manager mutex
   std::recursive_mutex manager_mutex_;
   // thread pool name
   std::string name_;
+  // map of paused tasks moved from worker queue that should not run at the time
+  std::map<TaskId, Worker<T>> paused_tasks_;

Review comment:
       there could be multiple tasks with the same `TaskId` when the maxConcurrentTasks is greater than 1, this way we could discard tasks on a pause + resume




----------------------------------------------------------------
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 pull request #974: MINIFICPP-1449 Add pause and resume command to C2

Posted by GitBox <gi...@apache.org>.
lordgamez commented on pull request #974:
URL: https://github.com/apache/nifi-minifi-cpp/pull/974#issuecomment-767526408


   C2PauseResumeTests should be okay now. After reproducing the issue and applying the change of the last commit, 1000 exections were run without failures on both ubuntu 20.04 and 16.04 distros. The last failure on windows-vs2017 is a know issue [MINIFICPP-1438](https://issues.apache.org/jira/browse/MINIFICPP-1438)


----------------------------------------------------------------
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 pull request #974: MINIFICPP-1449 Add pause and resume command to C2

Posted by GitBox <gi...@apache.org>.
lordgamez commented on pull request #974:
URL: https://github.com/apache/nifi-minifi-cpp/pull/974#issuecomment-758556854


   The C2PauseResumeTest is flaky because of the possible timings of the GET request and the HeartBeat messages, so it still needs to be stabilized.


----------------------------------------------------------------
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 pull request #974: MINIFICPP-1449 Add pause and resume command to C2

Posted by GitBox <gi...@apache.org>.
lordgamez commented on pull request #974:
URL: https://github.com/apache/nifi-minifi-cpp/pull/974#issuecomment-759493161


   @adamdebreceni @arpadboda I changed the semantics to stop the thread pool when we pause and restart again when we resume in [89ea022](https://github.com/apache/nifi-minifi-cpp/pull/974/commits/89ea02226dd48fcd288ce82354fb9e6ba8125b74). I misunderstood a previous comment as previously I stopped the ThreadedScheduler not the ThreadPool. This time it works by only stopping the ThreadPool.


----------------------------------------------------------------
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 #974: MINIFICPP-1449 Add pause and resume command to C2

Posted by GitBox <gi...@apache.org>.
szaszm commented on pull request #974:
URL: https://github.com/apache/nifi-minifi-cpp/pull/974#issuecomment-766801370


   Please don't forget to update the [C2 Design](https://cwiki.apache.org/confluence/display/MINIFI/C2+Design) wiki page.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi-minifi-cpp] arpadboda commented on pull request #974: MINIFICPP-1449 Add pause and resume command to C2

Posted by GitBox <gi...@apache.org>.
arpadboda commented on pull request #974:
URL: https://github.com/apache/nifi-minifi-cpp/pull/974#issuecomment-764799268


   I think we should investigate the CI failure here:
   
   ```
   C2PauseResumeTest: /home/runner/work/nifi-minifi-cpp/nifi-minifi-cpp/extensions/http-curl/tests/HTTPHandlers.h:432: void HeartbeatHandler::verify(mg_connection*): Assertion `ok' failed.
   ```


----------------------------------------------------------------
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 pull request #974: MINIFICPP-1449 Add pause and resume command to C2

Posted by GitBox <gi...@apache.org>.
lordgamez commented on pull request #974:
URL: https://github.com/apache/nifi-minifi-cpp/pull/974#issuecomment-766894988


   > > I think we should investigate the CI failure here:
   > > ```
   > > C2PauseResumeTest: /home/runner/work/nifi-minifi-cpp/nifi-minifi-cpp/extensions/http-curl/tests/HTTPHandlers.h:432: void HeartbeatHandler::verify(mg_connection*): Assertion `ok' failed.
   > > ```
   > 
   > I could not reproduce the issue locally after a few hundred runs with Release and Debug builds. I checked the failed assertion and it is a rapidjson parse failure of either an acknowledge or a heartbeat message sent by the Minifi process. It may have been a corrupted message due to some environment related issue, I'm not sure of the root cause, but if it resurfaces we could open an new issue to the transient failures epic.
   
   The issue seems to be reappearing, needs further investigation.


----------------------------------------------------------------
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 pull request #974: MINIFICPP-1449 Add pause and resume command to C2

Posted by GitBox <gi...@apache.org>.
lordgamez commented on pull request #974:
URL: https://github.com/apache/nifi-minifi-cpp/pull/974#issuecomment-766708979


   > I think we should investigate the CI failure here:
   > 
   > ```
   > C2PauseResumeTest: /home/runner/work/nifi-minifi-cpp/nifi-minifi-cpp/extensions/http-curl/tests/HTTPHandlers.h:432: void HeartbeatHandler::verify(mg_connection*): Assertion `ok' failed.
   > ```
   
   I could not reproduce the issue locally after a few hundred runs with Release and Debug builds. I checked the failed assertion and it is a rapidjson parse failure of either an acknowledge or a heartbeat message sent by the Minifi process. It may have been a corrupted message due to some environment related issue, I'm not sure of the root cause, but if it resurfaces we could open an new issue to the transient failures epic.


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