[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #735: MINIFICPP-1158 - Event driven processors can starve each other

[GitBox <gi...@apache.org>]

szaszm commented on a change in pull request #735: MINIFICPP-1158 - Event driven processors can starve each other
URL: https://github.com/apache/nifi-minifi-cpp/pull/735#discussion_r378946607
 
 

 ##########
 File path: libminifi/src/EventDrivenSchedulingAgent.cpp
 ##########
 @@ -34,26 +31,21 @@ namespace minifi {
 
 uint64_t EventDrivenSchedulingAgent::run(const std::shared_ptr<core::Processor> &processor, const std::shared_ptr<core::ProcessContext> &processContext,
                                          const std::shared_ptr<core::ProcessSessionFactory> &sessionFactory) {
-  while (this->running_) {
-    bool shouldYield = this->onTrigger(processor, processContext, sessionFactory);
-
-    if (processor->isYield()) {
-      // Honor the yield
-      return processor->getYieldTime();
-    } else if (shouldYield && this->bored_yield_duration_ > 0) {
-      // No work to do or need to apply back pressure
-      return this->bored_yield_duration_;
-    }
-
-    // Block until work is available
-
-    processor->waitForWork(1000);
-
-    if (!processor->isWorkAvailable()) {
-      return 1000;
+  if (this->running_ && processor->isRunning()) {
+    auto start_time = std::chrono::steady_clock::now();
+    // trigger processor until it has work to do, but no more than half a sec
+    while (std::chrono::steady_clock::now() - start_time < std::chrono::milliseconds(500)) {
+      bool shouldYield = this->onTrigger(processor, processContext, sessionFactory);
+      if (processor->isYield()) {
+        // Honor the yield
+        return processor->getYieldTime();
+      } else if (shouldYield) {
+        // No work to do or need to apply back pressure
+        return (this->bored_yield_duration_ > 0) ? this->bored_yield_duration_ : 10;  // No work left to do, stand by
+      }
     }
   }
-  return 0;
+  return 10;  // Let's check back for work in 10ms or when a thread is available to execute
 
 Review comment:
   Could you explain the reason behind the changes? It's not obvious from either the code or the PR title why this is necessary.
   
   The meaning of the return value is not obvious. Judging from the last comment, it's a duration in milliseconds. I know the lack of comment is not your fault, but I'd appreciate if you could comment its meaning somewhere near the function.
   
   There are too many hardcoded durations (10ms, 500ms). Could you explain why these were chosen?

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


With regards,
Apache Git Services