Github user jvwing commented on the pull request:
https://github.com/apache/nifi/pull/239#issuecomment-221619193
@mans2singh, I'm skeptical about the need to change the class hierarchy for
all AWS processors. I understand you want to share a common base class for the
Kinesis processors, and use shared AWS code for credentials, property
validation, etc. I also see that AbstractProcessor's functionality is not
particularly hard to replicate right now. But that might change in the future,
and most of the AWS processors would benefit from common functionality and
compliance without benefiting from the customization.
These Kinesis processors seem more of an exception to the rule rather than
an indicator of the common needs of AWS processors. As you point out above,
the Kinesis producer/consumer processors use a different set of AWS libraries,
running an out-of-process native code module, and driven by different
concurrency and flow control concerns. I don't believe these requirements will
be shared by any other AWS processors on the near horizon.
I don't have any big concerns about your implementation of
AbstractBaseAWSProcessor, it appears OK. Our AWS processor class hierarchy is
already in need of some repairs, and this could be made to fit. But I'm not
sure that should be done for this PR, driven by the Kinesis processors.
A few other comments:
- I recommend renaming the processors something like "GetKinesisStream" and
"PutKinesisStream", to distinguish them from PutKinesisFirehose and possible
future Kinesis processors for their analytics product.
- We should document the AWS permission requirements, at least a link to
the AWS docs on the permissions required by the KCL/KPL (Kinesis, DynamoDB, and
CloudWatch?).
- There does not appear to be a lot of unit test coverage of key onTrigger
methods and flowfile processing.
I am still working on running the integration tests and doing more detailed
code review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
