nifi-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jvwing <...@git.apache.org>
Subject [GitHub] nifi pull request: Nifi 1540 - AWS Kinesis Get and Put Processors
Date Wed, 25 May 2016 15:48:54 GMT
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message