NIFI-10500: Improved property ordering in MQTT processors#6417
NIFI-10500: Improved property ordering in MQTT processors#6417turcsanyip wants to merge 2 commits intoapache:mainfrom
Conversation
There was a problem hiding this comment.
Thank you for your changes, @turcsanyip. I've found a few minor things that could be improved. Could you check them?
| PROP_TOPIC, | ||
| PROP_RETAIN, | ||
| PROP_QOS, | ||
| RECORD_READER, |
There was a problem hiding this comment.
I'd probably put reader/writer properties to the end of the list to keep the mqtt related properties together.
There was a problem hiding this comment.
There are 2 main principles when ordering properties:
- properties with higher importance / being used frequently should come first
- keep related properties together
I believe the Timeout and Last Will properties are the least frequently used ones. Also, the by default visible Record properties may encourage their use. For these reasons I would keep this order even if those properties rather belong to the MQTT config. Record Writer has a similar position in the List* processors.
| if ((readerIsSet && !writerIsSet) || (!readerIsSet && writerIsSet)) { | ||
| results.add(new ValidationResult.Builder().subject("Reader and Writer").valid(false) | ||
| results.add(new ValidationResult.Builder().subject("Record Reader and Writer").valid(false) | ||
| .explanation("both Record Reader and Writer must be set when used.").build()); |
There was a problem hiding this comment.
Rendered text on the UI looks like "Record Reader and Writer is invalid, because both Record Reader and Writer must be set when used." which is a bit verbose. I'd remove the reference to "Record Reader and Writer" from the explanation to make the description simpler.
| .name("add-attributes-as-fields") | ||
| .displayName("Add attributes as fields") | ||
| .description("If using the Records reader/writer and if setting this property to true, default fields " | ||
| .description("If using the Record Reader/Writer and if setting this property to true, default fields " |
There was a problem hiding this comment.
Because of the dependsOn property, the first if will always be true. I'd remove it to make the description simpler.
| PROP_GROUPID, | ||
| PROP_TOPIC_FILTER, | ||
| PROP_QOS, | ||
| PROP_MAX_QUEUE_SIZE, |
There was a problem hiding this comment.
I'd put this property to the end of the list (before reader/writers if you accept that comment) because this property is not strictly related to the MQTT client itself as the other properties around it.
nandorsoma
left a comment
nandorsoma
left a comment
There was a problem hiding this comment.
Thanks @turcsanyip, LGTM!
|
Merged in 5303aad |
3 participants
Summary
NIFI-10500
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation