NIFI-14109 Refactored remaining processors and control services to be uniform when creating properties and relationships.#9600
Conversation
| relationships.addAll(getAdditionalRelationships()); | ||
| this.relationships = Collections.unmodifiableSet(relationships); | ||
| this.descriptors = Stream.concat( | ||
| PROPERTIES.stream(), |
There was a problem hiding this comment.
Is there no simpler/cleaner option than Stream.concat ... list.stream().... .toList()? Overall this is cleanear in some ways but does seem like there is a cleaner/simpler way of saying 'List from these lists'
There was a problem hiding this comment.
I used this as I saw that being used in other parts of the code e.g. ElasticSearchLookupService.
There was a problem hiding this comment.
It would surprise me if this is the best way to express these declarations.
There was a problem hiding this comment.
I thought it was the cleanest way to make an unmodifiable list. A quick Google search indicates that is the Java 9 way to combine lists and make them unmodifiable.
...rocessors/src/main/java/org/apache/nifi/services/azure/eventhub/AzureEventHubRecordSink.java
Outdated
Show resolved
Hide resolved
| AMQP_VERSION, | ||
| SSL_CONTEXT_SERVICE, | ||
| USE_CERT_AUTHENTICATION | ||
| ); |
There was a problem hiding this comment.
Here the parent class 'getCommonPropertyDescriptors' method is removed and instead a subclass is supposed to know to use the static list of descriptors as they build their own lists. I think this weakens what the author intended in building that class.
Also there is at least one example much later on in this PR where you kept the parent method. I think keeping the parent method better conveys intent to any subclassers.
There was a problem hiding this comment.
I will revert this back.
There was a problem hiding this comment.
I ended up adding 'getCommonPropertyDescriptors' for all the parent children relationships I saw. Below are the ones I made changes for (Parents on the left and children who use the method on the right).
- AbstractAMQPProcessor - ConsumeAMQP, PublishAMQP
- AbstractAwsMachineLearningJobStarter - StartAwsTextractJob
- AbstractAwsMachineLearningJobStatusProcessor - GetAwsTextractJobStatus
- AbstractAzureCosmosDBProcessor - PutAzureCosmosDBRecord
- AbstractEmailProcessor - ConsumeIMAP, ConsumePOP3
- AbstractGridFSProcessor (applies this also to relationships) - FetchGridFS, PutGridFS
- AbstractHadoopProcessor - AbstractFetchHDFSRecord, AbstractPutHDFSRecord (PutParquet but doesn't use common method), CreateHadoopSequenceFile, DeleteHDFS, FetchHDFS, GetHDFS, GetHDFSEvents, GetHDFSFileInfo, ListHDFS, MoveHDFS, PutHDFS
- AbstractJoltTransform - JoltTransformJSON, JoltTransformRecord
- AbstractMongoProcessor - DeleteMongo, GetMongo, PutMongo, PutMongoBulkOperations, PutMongoRecord, RunMongoAggregation
- SplunkAPICall - PutSplunkHTTP, QuerySplunkIndexingStatus
dan-s1
force-pushed
the
NIFI-14109
branch
2 times, most recently
from
c05a2c0 to
d5e7a3b
Compare
exceptionfactory
left a comment
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for working on improving the consistency of approach @dan-s1.
I'm in favor of going with PROPERTY_DESCRIPTORS and RELATIONSHIPS as currently implemented. If you can rebase the pull request, this looks close to completion.
… uniform when creating properties and relationships.
…ROPERTY_DESCRIPTORS. For parent processor classes which defined common properties, made getCommonPropertyDescriptors() method for children to use to load them. Ensured lists of PropertyDescriptor objects has each PropertyDescriptor on its own line and sets of RelationShip objects has each Relationship on its own line to improve readability.
exceptionfactory
left a comment
exceptionfactory
left a comment
There was a problem hiding this comment.
I'm in the favor of the current set of changes, as it includes a notable clean up of property descriptor and relationship collection construction.
Do you have any additional feedback @joewitt?
|
It is a massive change set so definitely don't feel super comfortable :) I also find the 'concat/streamof' to be not very nice but that might just be me. If we are good then yolo! i'm fine |
Yea, I agree the Stream.concat() is less optimal than the pure List.of(), but it reflects some of the property descriptor inheritance currently in play. That in itself is not always optimal, but I think that is outside of this PR, and the Stream.concat() is marginally better than repetitive add() calls on an intermediate list. Thanks for the feedback! |
3 participants
Summary
NIFI-14109
This PR aims to have a lists of
PropertyDescriptorobjects and sets ofRelationshipobjects all be defined with onePropertyDescriptorandRelationshipper line for increased readability. The list variables are namedPROPERTY_DESCRIPTORSand the set variables are namedRELATIONSHIPS. Also for commonPropertyDescriptorobjects defined in a parent a method, a methodgetCommonPropertyDescriptorsis defined to allow children to use them. This was done for the following parent and children classes (parents on left and children which use the method are on the right).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