[GitHub] nifi issue #1125: NIFI-2872 Create PutCloudwatchMetric Processor
Github user jvwing commented on the issue: https://github.com/apache/nifi/pull/1125 I expect so :). The latest changes look good, thanks for those. I'm going to do a bit more testing, and if everything goes well, I'll merge it this evening. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi issue #1125: NIFI-2872 Create PutCloudwatchMetric Processor
Github user vegaed commented on the issue: https://github.com/apache/nifi/pull/1125 So I guess it ready to be merged then? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi issue #1125: NIFI-2872 Create PutCloudwatchMetric Processor
Github user jvwing commented on the issue: https://github.com/apache/nifi/pull/1125 @vegaed I don't think you should worry, I do not believe the Appveyor build is stable. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi issue #1125: NIFI-2872 Create PutCloudwatchMetric Processor
Github user vegaed commented on the issue: https://github.com/apache/nifi/pull/1125 @jvwing You know why the appveyor build is failing. I didn't touch any of the S2S code. Is it something I should worry about? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi issue #1125: NIFI-2872 Create PutCloudwatchMetric Processor
Github user vegaed commented on the issue: https://github.com/apache/nifi/pull/1125 @jvwing Thanks for all the great suggestions and code. I moved the try up in order to catch those types of parsing exceptions so now they will route to failure. Good catch on the integration tests Yes I agree unit testing is good. I was modeling off of S3 and SQS and didn't see any there, but I see now that dynamo and kinesis packages are being both unit and integration tested. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi issue #1125: NIFI-2872 Create PutCloudwatchMetric Processor
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/1125
@vegaed Thanks for the updated code. The situation with
AbstractAWSProcessor is a bit odd. It's deprecated in that you should not
derive directly from it, but it still has live code we're definitely using.
* **Number Parsing Exception Handling** - I tested with a non-double metric
value. A java.lang.NumberFormatException is thrown parsing the number at
PutCloudWatchMetric.java:174. The error message is clear enough, but the
exception is not caught inside onTrigger(), so the entire session is rolled
back and the flowfile remains in the incoming queue. The same thing applies to
parsing Timestamp. Shouldn't we catch the exception and route the flowfile to
failure in a case like this?
* **Integration Tests** - Integration tests failed for me, complaining
about no flowfiles routed to REL_SUCCESS. I believe the fix might be as simple
as adding an empty flowfile before running:
runner.enqueue(new byte[] {});
runner.run();
* **Unit Tests** - How about some unit tests that do not call AWS? I agree
there is no substitute for integration tests that actually call AWS, but I
believe there is also no substitute for unit tests that can easily be run by
anyone and everyone. This can be a bit tricky for cloud services, but it seems
like CloudWatch has an API with nice request/response patterns that can be
mocked. So I tried my hand at a [simple mock setup with sample tests for
PutCloudWatchMetric](https://github.com/jvwing/nifi/commit/b6ae1accfecb464fe16a456cdef04669a65672fe).
You're welcome to not use this, but I recommend we try something similar that
gets coverage of onTrigger from unit tests.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1125: NIFI-2872 Create PutCloudwatchMetric Processor
Github user vegaed commented on the issue: https://github.com/apache/nifi/pull/1125 @jvwing Thanks for the review. Just fixed those issues up. Only reason I didn't use the RELs from AbstractAWSProcessor is because it is a deprecated class, but if it standard practice. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi issue #1125: NIFI-2872 Create PutCloudwatchMetric Processor
Github user jvwing commented on the issue: https://github.com/apache/nifi/pull/1125 @vegaed thanks for putting this together. I have a few initial suggestions from a quick scan: * Please run `mvn clean install -Pcontrib-check` - there are a few issues with .* imports, unused imports, etc. * Did you forget to incude an entry for PutCloudWatchMetric processor in the service definition file org.apache.nifi.processor.Processor? * For PropertyDescriptors, please provide both `name` (as a data key) and `displayName` (for UI presentation) * Why not use REL_SUCCESS and REL_FAILURE inherited from AbstractAWSProcessor? The descriptions would be less specific, but it appears to be a standard practice. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
