[GitHub] [nifi-minifi-cpp] dam4rus commented on pull request #992: MINIFICPP-1467 Remove mutex from Connection

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

dam4rus commented on pull request #992:
URL: https://github.com/apache/nifi-minifi-cpp/pull/992#issuecomment-780465445


   I agree that this implementation doesn't fit the current design but to avoid needless lockings and potentially tank the performance I choose this over `push_all`, `pop_all`
   
   For example:
   At https://github.com/apache/nifi-minifi-cpp/pull/992/files#diff-edfe02f672dad6c32d860c97ea73e5f4b95889387942c8b372554ba43fda14cdL159 `flows` are filtered so I would need to create a new container. It's not a big issue since `shared_ptr` is fairly cheap to copy but nonetheless felt unnecessary. Also, what would be the implementation of `push_all`? Should it accept `std::vector`, `std::queue` or iterator pair?
   
   But the biggest issue is here https://github.com/apache/nifi-minifi-cpp/pull/992/files#diff-edfe02f672dad6c32d860c97ea73e5f4b95889387942c8b372554ba43fda14cdL181 , since there's both pop and push in the same loop if the flow file is penalised, which breaks the loop and put's the flow file back into the queue. Maybe `pop`-ing should be moved to the end of the loop to avoid an unnecessary `pop` and `push` if the flow file is penalised? So it's not really a `pop_all`. So without exposing the underlying queue somehow, this would require some refactoring and/or hurting the performance by locking the mutex with every call to `ConcurrentQueue`'s methods.


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