MINIFICPP-1440 PublishKafka processor's "security protocol" should ha…#961
MINIFICPP-1440 PublishKafka processor's "security protocol" should ha…#961dam4rus wants to merge 2 commits intoapache:mainfrom
Conversation
…ve allowable values
szaszm
left a comment
szaszm
left a comment
There was a problem hiding this comment.
Thanks for your first contribution. We generally merge PRs after 2-3 approvals and when all comments are resolved. In case there are any questions, feel free to reach out.
| const core::Property PublishKafka::SecurityProtocol( | ||
| core::PropertyBuilder::createProperty("Security Protocol") | ||
| ->withDescription("Protocol used to communicate with brokers") | ||
| ->withAllowableValues<std::string>({SECURITY_PROTOCOL_PLAINTEXT, SECURITY_PROTOCOL_SSL, SECURITY_PROTOCOL_SASL_PLAINTEXT, SECURITY_PROTOCOL_SASL_SSL}) |
There was a problem hiding this comment.
When looking at PublishKafka.cpp:686, it seems like only SECURITY_PROTOCOL_SSL is actually supported, and the default empty value is interpreted as plaintext.
I suggest restricting the set of "allowable values" to plaintext and ssl, and handling plaintext near PublishKafka.cpp:687. I think doing nothing is a valid handling of plaintext.
There was a problem hiding this comment.
I would also add a default value for PLAINTEXT as we are trying to make the behaviour consistent with NiFi:
https://nifi.apache.org/docs/nifi-docs/components/nifi-docs/components/org.apache.nifi/nifi-kafka-2-0-nar/1.9.0/org.apache.nifi.processors.kafka.pubsub.PublishKafka_2_0/
There was a problem hiding this comment.
When looking at
PublishKafka.cpp:686, it seems like onlySECURITY_PROTOCOL_SSLis actually supported, and the default empty value is interpreted as plaintext.
Should I leave the unused defines in? If it doesn't breaks backward compatibility removing them would make the code a bit clearer.
I suggest restricting the set of "allowable values" to plaintext and ssl, and handling plaintext near PublishKafka.cpp:687. I think doing nothing is a valid handling of plaintext.
What's the suggested coding style in this case? Having an empty else if (value == SECURITY_PROTOCOL_PLAINTEXT) branch or just using else if (value != SECURITY_PROTOCOL_PLAINTEXT) to handle unexpected values?
I would also add a default value for PLAINTEXT as we are trying to make the behaviour consistent with NiFi:
https://nifi.apache.org/docs/nifi-docs/components/nifi-docs/components/org.apache.nifi/nifi-kafka-2-0-nar/1.9.0/org.apache.nifi.processors.kafka.pubsub.PublishKafka_2_0/
Nifi also describes it as a required value. Should I add that too?
There was a problem hiding this comment.
- Feel free to delete unused parts.
- Up to you. If you feel fancy, you can extend the property API to be able to return the allowable values and check against presence there.
- Yes. The more important thing is that we are compatible with NiFi configs, but compatibility in the other direction is probably nice to have.
…ve allowable values
4 participants
…ve allowable values
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.