-
Notifications
You must be signed in to change notification settings - Fork 2.9k
NIFI-15017 Add start/stop C2 command for processors in MiNiFi #10353
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
base: main
Are you sure you want to change the base?
NIFI-15017 Add start/stop C2 command for processors in MiNiFi #10353
Conversation
22e7ed4
to
6e15b54
Compare
return FULLY_APPLIED; | ||
} catch (Exception e) { | ||
LOGGER.error("Failed to change state for processor {}", processorId, e); | ||
return PARTIALLY_APPLIED; |
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.
In my opinion the PARTIALLY_APPLIED result is not suitable for a single processor start/stop operation. It would make sense for a processor group what could be partially started or stopped (eg some of the processors in the group are started or stopped by the operation meanwhile others remain in the opposite state for some reason) but the operation result for a single processor should not be partial.
@lordgamez could you please check what could be the operation state from the CPP agent in case of a processor start/stop operation? We should keep this in line with the existing implementation in the CPP agent.
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.
According to the cpp implementation ( response construction for start/stop and c2payload implementation ) it seems the response to a successful start/stop is a FULLY_APPLIED
status. As a result i removed the PARTIALLY_APPLIED
response as you suggested but still left the NOT_APPLIED
in case of error. From my understanding of the cpp code there doesnt seem to be any handling of the case where the start/stop operation fails.
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.
Thx
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.
@pkedvessy sorry I could not check the change earlier, @Mihhai is right we only respond with FULLY_APPLIED
every time for the start/stop messages, thanks for adjusting the code to minificpp!
return result; | ||
} | ||
|
||
private RunStatus mapProcessorRunStatus(org.apache.nifi.controller.status.RunStatus controllerRunStatus) { |
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.
In the switch statement, we can simply return STOPPED as default and we can keep RUNNING mapping as it is now.
In other hand, controllerRunStatus parameter naming is misleading. We should use something like a runStatus or processorRunStatus as parameter name or we can simply set the runStatus without this method like
result.setRunStatus(processorStatus.getRunStatus() == Running ? RUNNING : STOPPED);
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 refactored to the one line mapping you suggested and i removed the helper method.
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.
Thx
3e2629b
to
f4e9486
Compare
return changeState(processorId, false); | ||
} | ||
|
||
private OperationState changeState(String processorId, boolean start) { |
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.
Nitpick:
Controlling the state transition with a boolean flag works but not well readable without checking the body of the method. I can see the reason behind: this way you did not duplicate the boilerplate code around the flowController calls. I think it would be a bit more readable by passing the state transition as a method reference:
Eg.: changeState(processorId, this::star) and changeState(processorId, this::stop)
private void start(String processorId, String parentGroupId) ...
private void stop(String processorId, String parentGroupId) ...
In general I'm happy with your PR but before moving forward to merge it I'd have someone from the CPP Agent team to make sure the C2 API is 100% compatible with their implementation. |
I believe it's currently not compatible with the API implemented by MiNiFi C++, but we're open to an API change if we can agree that the new API is an improvement, and if we can provide a transition period. I find it difficult to understand what protocol API this code maps to. In MiNiFi C++, I believe we have this:
From what I understand after talking to @pkedvessy, this pull request adds a new "name" called "processor, where we would put the processor UUID in the (currently unused) arguments, as a JSON Object with the key "processorId". |
Just to have a clear example currently in MiNiFi C++ the format of the start/stop request looks like the following where the operand is a processor id:
What is the equivalent json in this case? |
Currently the json for a stop flow opearation which is already integrated in the java solution would be :
I believe this stops everything inside the root process group, and if anything other than FLOW is sent as operand the command fails The json for the stop processor command i proposed would look like this:
This is the implementation i naturally gravitated towards given the selection machanism of handlers that the java implementation has. It selects the proper handler based on a combination of operation/operand. I believe there are already some differences between the interfaces of the 2 implementations.So in order to be compatible with the cpp variant and have start/stop per processor we would need some sort of detection logic for the value received in the operand field. |
I think this is okay to move forward with this implementation and we'll adjust minifi-cpp and the open source minifi-c2 server implementation to this in the future. It will be easier to accept both the old and the new notation in minifi-cpp for a while until the old notation is taken out in a future release. |
I agree with Gábor: we can go forward with this API and change minifi c++ and ask adamdebreceni/c2-server to use this new API. It seems to work better in the Java world to have the operand name be a static string. @Mihhai just out of curiosity, what C2 server do you use? Do you have a custom one, or did you patch something to add this capability? If it's open source, we may want to add integration tests with it to ensure no breakage in the future. |
I just implemented a small, custom server to exercise and validate the flows during development. Nothing special, i just exposed 2 endpoints (heartbeat and acknowledgment) and some simple mechanism to queue up commands in the heartbeat response |
Summary
NIFI-15017
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000
NIFI-00000
Pull Request Formatting
main
branchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-check
Licensing
LICENSE
andNOTICE
filesDocumentation