MINIFICPP-1390 Create DeleteS3Object processor by lordgamez · Pull Request #931 · apache/nifi-minifi-cpp

lordgamez · 2020-10-27T17:03:42Z

Jira issue: https://issues.apache.org/jira/browse/MINIFICPP-1390

Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit?

For code changes:

If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE file?
If applicable, have you updated the NOTICE file?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.

arpadboda

Mostly looks good, added some minor comments.

extensions/aws/processors/DeleteS3Object.cpp

extensions/aws/s3/S3Wrapper.cpp

adamdebreceni · 2020-11-18T14:25:26Z

wrapping the object id in quotes when logging might improve readability of the log message (might be beneficial for the bucket id as well) (here "one" is the object id)

::S3WrapperBase] [info] Deleted S3 object one from bucket adebreceni-minifi-0

hunyadi-dev · 2020-12-09T08:43:06Z

PROCESSORS.md

+
+### Description
+
+Deletes FlowFiles on an Amazon S3 Bucket. If attempting to delete a file that does not exist, FlowFile is routed to success.


This sounds like "if something does not exist it is rerouted to success".

I see this is verbatim from NiFi, but I still don't understand what is going on. If a flowfile does not exist, this processor yields.

We started discussing this with @arpadboda and @adebreceni in a comment above, where @adebreceni noted that it can be logical if we interpret it the following way: "if you define "success" as the post-condition "the object is not in the S3 bucket", it makes sense to transfer to "success"". So if the goal is to have the object non-existent in that bucket we succeed by doing nothing.

hunyadi-dev · 2020-12-09T08:50:37Z

extensions/aws/processors/DeleteS3Object.cpp

+
+  if (!getExpressionLanguageSupportedProperties(context, flow_file)) {
+    context->yield();
+    return;


Shouldn't the flowfile get transfered to Failure here?

I think you are right, as if this fails we cannot continue with this flow file as one of the required properties is missing.

Fixed in af2501b

hunyadi-dev · 2020-12-09T08:59:17Z

extensions/aws/processors/DeleteS3Object.cpp

+
+void DeleteS3Object::initialize() {
+  // Set the supported properties
+  std::set<core::Property> properties(S3Processor::getSupportedProperties());


Minor, but instead of adding the properties manually and defining them twiceS3Processor could set its supported properties in its constructor, and the interface for the ConfigurableComponent could be extended with insertion for new properties.

Included in b065515. The only difference is that I did not introduce a new insertion operation in the ConfigurableComponent but used updateSupportedProperties instead.

hunyadi-dev · 2020-12-09T09:06:42Z

extensions/aws/processors/DeleteS3Object.h

+    : S3Processor(std::move(name), uuid, logging::LoggerFactory<DeleteS3Object>::getLogger()) {
+  }
+
+  explicit DeleteS3Object(std::string name, minifi::utils::Identifier uuid, std::unique_ptr<aws::s3::S3WrapperBase> s3_wrapper)


Is this constructor used somewhere?

Yes, this constructor is used in the tests to pass the mocked version of the S3 wrapper.

if it is only used for testing we could make it private and friend a DeleteS3ObjectTestAccessor (or some other equally horrendously named class)

Moved the constructor private in b04bddf

hunyadi-dev · 2020-12-09T09:09:11Z

extensions/aws/processors/S3Processor.cpp

+  core::PropertyBuilder::createProperty("Communications Timeout")
+    ->isRequired(true)
+    ->withDefaultValue<core::TimePeriodValue>("30 sec")
+    ->withDescription("")


I see that NiFi does not have this either, but shouldn't we add some explanation?

Added description in 4f3ac8e

hunyadi-dev · 2020-12-09T09:10:25Z

extensions/aws/processors/S3Processor.cpp

+    EndpointOverrideURL, ProxyHost, ProxyPort, ProxyUsername, ProxyPassword, UseDefaultCredentials};
+}
+
+minifi::utils::optional<Aws::Auth::AWSCredentials> S3Processor::getAWSCredentialsFromControllerService(const std::shared_ptr<core::ProcessContext> &context) const {


Minor, but minifi:: seems redundant.

It is needed in this case, as we introduced minifi::aws::utils and this class is in the scope of the aws namespace.

lordgamez force-pushed the MINIFICPP-1390 branch from 58d38e7 to 9fceb84 Compare November 9, 2020 12:04

arpadboda reviewed Nov 11, 2020

View reviewed changes

lordgamez force-pushed the MINIFICPP-1390 branch from 9fceb84 to a2ba9ac Compare November 11, 2020 16:38

lordgamez force-pushed the MINIFICPP-1390 branch from 5009b52 to ccdde78 Compare November 25, 2020 14:35

adamdebreceni approved these changes Dec 1, 2020

View reviewed changes

lordgamez force-pushed the MINIFICPP-1390 branch from ccdde78 to 774166e Compare December 8, 2020 08:51

hunyadi-dev reviewed Dec 9, 2020

View reviewed changes

lordgamez force-pushed the MINIFICPP-1390 branch 2 times, most recently from 4f3ac8e to 3f872f1 Compare December 11, 2020 13:43

lordgamez added 15 commits January 5, 2021 10:24

Extract common S3 functionality from PutS3Object

e8e8b4e

Add DeleteS3Object implementation and tests

407415e

Extract common S3 test fixture

13137b4

Add failure case for delete S3 tests

0b91556

Add docker integration tests

af82933

Check EL support on version

27febc1

Test delete of non existing S3 object

132aced

Add documentation for DeleteS3Object

f5466c3

Move instead of copy common parameters to put object parameters

d146862

Fix linter issues

7a78e2e

Add DeleteS3Object to AWS Loader

6e18f04

Add common tests to DeleteS3ObjectTests

96e8cee

Do not set empty version

e9e7f60

Additional tests

055b7dc

Fix PROCESSORS.md

3e35af2

lordgamez added 14 commits January 5, 2021 10:24

Refactor S3Processor

7a842c5

Refactor MockS3Wrapper

e3a6284

Remove unnecessary yield

0d6638b

Minor fixes in tests

d0a8cfc

Make version in delete optional

75d8781

Refactor S3Processor constructors

5950afa

Remove unnecessary includes

3c74e71

Fix review comments

5d65789

Add S3 processors to README.md

50df9bc

Improve logging

2f32ccc

Transfer flowfiles to failure in S3 processor if properties missing

4e3e4ca

Remove redundant getSupportedProperties member function

4823bb6

Make S3 processor alternative constructor only accessible by tests

37120c8

Add documentation for communications timeout

6b3120a

lordgamez force-pushed the MINIFICPP-1390 branch from 3f872f1 to 6b3120a Compare January 5, 2021 09:25

arpadboda closed this in 07781b8 Jan 5, 2021


		### Description

		Deletes FlowFiles on an Amazon S3 Bucket. If attempting to delete a file that does not exist, FlowFile is routed to success.

Conversation

lordgamez commented Oct 27, 2020

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

arpadboda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamdebreceni commented Nov 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lordgamez Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamdebreceni Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hunyadi-dev Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lordgamez Dec 11, 2020 •

edited

Loading

adamdebreceni Dec 11, 2020 •

edited

Loading

hunyadi-dev Dec 9, 2020 •

edited

Loading