-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MINIFICPP-2377 Support process group level controller services #1840
base: main
Are you sure you want to change the base?
Conversation
@@ -200,7 +212,13 @@ void ProcessGroup::startProcessing(TimerDrivenSchedulingAgent& timeScheduler, Ev | |||
|
|||
void ProcessGroup::stopProcessing(TimerDrivenSchedulingAgent& timeScheduler, EventDrivenSchedulingAgent& eventScheduler, | |||
CronDrivenSchedulingAgent& cronScheduler, const std::function<bool(const Processor*)>& filter) { | |||
std::lock_guard<std::recursive_mutex> lock(mutex_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and similar changes in the startProcessingProcessors
and startProcessing
functions are to avoid deadlock. It appeared in ControllerServiceIntegrationTests
when stop()
is called while the getControllerService
is called in the processor's onTrigger
. The stop()
function waits for the processor's onTrigger
to finish, but it cannot finish the ProcessGroup's findProcessor
method because it is waiting for the same mutex which is held here.
We may need to find a better solution as this may cause some concurrency issues I'm not currently aware of. I'm open for suggestions.
cc6f234
to
2eaeafc
Compare
2eaeafc
to
7a9383a
Compare
7a9383a
to
852793c
Compare
template<typename T = core::Processor> | ||
T* getProcessor() const { return static_cast<T*>(processor_); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with this change, this class shouldn't expose its processor. The old pattern works fine, and keeps this utility simpler. And reverting this part would also make the diff smaller.
I like the move over to unique_ptr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the change of adding the unique_ptr of the root process group in the test plan and that root process group owning the processors and the controllers, retrieving the pointers to the processors became problematic in some cases. For example in test suite initialization:
PushGrafanaLokiGrpcTestFixture()
: mock_loki_("10991"),
test_controller_(std::make_unique<PushGrafanaLokiGrpc>("PushGrafanaLokiGrpc")),
push_grafana_loki_grpc_(test_controller_.getProcessor<PushGrafanaLokiGrpc>()) {
In this case initializing the processor pointer would be problematic without the getProcessor
method, we would need to initialize them separately afterwards in the main constructor block.
Also I think it would make the diff larger, as the common pattern which currently looking like this:
test::SingleProcessorTestController test_controller{std::make_unique<InvokeHTTP>("InvokeHTTP")};
auto invokehttp = test_controller.getProcessor();
would probably need to be changed to something like this:
auto processor = std::make_unique<InvokeHTTP>("InvokeHTTP");
auto invokehttp = processor.get();
test::SingleProcessorTestController test_controller{std::move(processor)};
If you think it's worth reverting and changing the initializations, I'm okay with that too, I just wanted to add some context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the idea of complicating TestBase or TestController to mirror the god class complexity of FlowController. I would rather keep the process groups and the processor ownership outside of that, in the test case scopes, if that's feasible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, we shouldn't have a god class like FlowController here, but I'm not sure this change complicates the previous behavior. Previously we also had all the controllers and processors as part of the TestPlan, the only difference is that the exclusive ownership is now in the test plan instead of the shared ownership between the TestPlan and the test case. The change had to be made because controller services are now only valid on a process group level and both the processors and controllers had to be added to the same process group in the test scenarios to be able to retrieve the controllers in the scope of a processor that defines a controller in their processor properties. I think this was the least intrusive change to be able to use the processors and controllers in our test framework, of course we can redesign the TestBase, TestController and TestPlan, but I think that's out of the scope of this PR.
852793c
to
19dca48
Compare
19dca48
to
c71a5d0
Compare
https://issues.apache.org/jira/browse/MINIFICPP-2377
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:
For documentation related changes:
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.