MINIFICPP-1396 Create FetchS3Object processor #970

lordgamez · 2021-01-05T14:54:11Z

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, some minor comments.

extensions/aws/processors/FetchS3Object.cpp

extensions/aws/processors/FetchS3Object.h

extensions/aws/s3/S3WrapperBase.cpp

adamdebreceni · 2021-01-06T09:42:40Z

extensions/aws/s3/S3WrapperBase.cpp

@@ -68,13 +75,13 @@ void S3WrapperBase::setCannedAcl(Aws::S3::Model::PutObjectRequest& request, cons
  request.SetACL(CANNED_ACL_MAP.at(canned_acl));
 }

-std::string S3WrapperBase::getExpiryDate(const std::string& expiration) {
+std::pair<std::string, std::string> S3WrapperBase::getExpirationPair(const std::string& expiration) {
  static const std::regex expr = std::regex("expiry-date=\"(.*)\", rule-id=\"(.*)\"");


shouldn't we use the Regex class defined in RegexUtils.h? (that contains some "magic" for <GCC 4.9)

Fixed in 8ddbc42

adamdebreceni · 2021-01-06T09:48:50Z

extensions/aws/s3/S3WrapperBase.cpp

@@ -68,13 +75,13 @@ void S3WrapperBase::setCannedAcl(Aws::S3::Model::PutObjectRequest& request, cons
  request.SetACL(CANNED_ACL_MAP.at(canned_acl));
 }

-std::string S3WrapperBase::getExpiryDate(const std::string& expiration) {
+std::pair<std::string, std::string> S3WrapperBase::getExpirationPair(const std::string& expiration) {


even though it is only used twice, we could create a struct (given how both members of the pair are of the same type)

Fixed in ad6ee95

adamdebreceni · 2021-01-06T11:40:28Z

extensions/aws/processors/FetchS3Object.cpp

+    return false;
+  }
+  get_object_params_.bucket = std::move(bucket_);
+  get_object_params_.object_key = std::move(object_key_);


it seems like object_key_ and bucket_ are a processor members, and we read/write them from possibly multiple concurrent onTriggers

the same goes for, I think, aws_credentials_provider_and s3_wrapper_ where we call S3WrapperBase::setProxy and S3WrapperBase::setCredentials from the onTriggers

Could you please elaborate on this a bit more? I am not that familiar with the scheduling of the onTrigger calls. Are you saying that it is possible that the same processor's onTrigger call can be scheduled on separate threads in the threadpool and depending on the timing they might overlap causing the concurrency?

Yes, this can happen depending on configurations.
In most of the processors we just prevent this by having a mutex for the whole ontrigger call, in this case we might just protect some common resources.

yes, as far as I know, onSchedule is guaranteed to be called once for a processor, but multiple onTriggers can run concurrently on multiple threads, the number of such onTriggers is controlled by the max concurrent tasks property of the processor

of course we could always just ignore this property and override getMaxConcurrentTasks to return 1 (or the mutex @arpadboda mentioned), but I don't (yet) think there is an inherent reason to do so, for this processor (family)

Thanks for the info and great catch! I removed the members that were modified in the onTrigger calls and added a mutex for the S3Wrapper calls to guarantee that the S3 configuration is not changed between AWS calls. Change is done in e59580a

hunyadi-dev · 2021-01-06T15:16:00Z

PROCESSORS.md

+|Secret Key|||AWS account secret key<br/>**Supports Expression Language: true**|
+|Credentials File|||Path to a file containing AWS access key and secret key in properties file format. Properties used: accessKey and secretKey|
+|AWS Credentials Provider service|||The name of the AWS Credentials Provider controller service that is used to obtain AWS credentials.|
+|**Region**|us-west-2|af-south-1<br/>ap-east-1<br/>ap-northeast-1<br/>ap-northeast-2<br/>ap-northeast-3<br/>ap-south-1<br/>ap-southeast-1<br/>ap-southeast-2<br/>ca-central-1<br/>cn-north-1<br/>cn-northwest-1<br/>eu-central-1<br/>eu-north-1<br/>eu-south-1<br/>eu-west-1<br/>eu-west-2<br/>eu-west-3<br/>me-south-1<br/>sa-east-1<br/>us-east-1<br/>us-east-2<br/>us-gov-east-1<br/>us-gov-west-1<br/>us-west-1<br/>us-west-2|AWS Region|


Very minor, but this list of allowable values does not look well at all when rendered:

It might be worth doing some manual tweaking on it, eg changing font size and adding some padding to the top and bottom. Same stands for DeleteS3Object.

I adjusted the cell width with a fix but scalable div in 9417f98. The region values should not wrap anymore.

hunyadi-dev · 2021-01-06T15:29:29Z

libminifi/test/aws-tests/FetchS3ObjectTests.cpp

+      core::Relationship("success", "d"),
+      true);
+    char output_dir_mask[] = "/tmp/gt.XXXXXX";
+    output_dir = test_controller.createTempDirectory(output_dir_mask);


You can get rid of the mask by using:

output_dir = utils::createTempDir(&testController);

Added in 7b57ab6

hunyadi-dev · 2021-01-06T15:40:05Z

libminifi/test/aws-tests/FetchS3ObjectTests.cpp

+TEST_CASE_METHOD(FetchS3ObjectTestsFixture, "Test default properties", "[awsS3Config]") {
+  setRequiredProperties();
+  test_controller.runSession(plan, true);
+  REQUIRE(LogTestController::getInstance().contains("key:s3.bucket value:" + S3_BUCKET));


Minor but contains is slow. utils::verifyLogLinePresenceInPollTime should be a better choice here.

Replaced it in 840a645.

hunyadi-dev · 2021-01-06T15:40:48Z

libminifi/test/aws-tests/FetchS3ObjectTests.cpp

+  REQUIRE(mock_s3_wrapper_ptr->getCredentials().GetAWSSecretKey() == "secret");
+}
+
+TEST_CASE_METHOD(FetchS3ObjectTestsFixture, "Test required property not set", "[awsS3Config]") {


👍 for proper negative tests

hunyadi-dev · 2021-01-06T15:48:19Z

libminifi/test/aws-tests/FetchS3ObjectTests.cpp

+  std::ifstream s3_file(output_dir + "/" + INPUT_FILENAME);
+  std::string output((std::istreambuf_iterator<char>(s3_file)), std::istreambuf_iterator<char>());
+  REQUIRE(output.empty());


Probably a good candidate to extract to FileUtils.h

Extraced in 6097258

hunyadi-dev

LGTM, only minor suggestions added.

arpadboda reviewed Jan 5, 2021

View reviewed changes

extensions/aws/processors/FetchS3Object.cpp Outdated Show resolved Hide resolved

extensions/aws/processors/FetchS3Object.h Show resolved Hide resolved

extensions/aws/s3/S3WrapperBase.cpp Outdated Show resolved Hide resolved

adamdebreceni reviewed Jan 6, 2021

View reviewed changes

lordgamez added 13 commits January 6, 2021 10:57

MINIFICPP-1396 Create FetchS3Object processor

2f1187f

Add integration tests for FetchS3Object

bab1d4b

Add FetchS3Object to PROCESSORS.md

2e72fb2

Make requester pays property mandatory

bd26b10

Rename input params of S3 operations

6298e3e

Add no object key set test

626fc07

Fix linter errors

16597b4

Rebase after recent changes and adjust credential testing

bc5ff85

Fix compilation after rebase

a9216a4

Incorporate review comments from earlier S3 processor reviews

f9e373f

Use split_path form FileUtils for FetchS3Object path processing

866f07a

Replace std::regex with RegexUtils

8ddbc42

Replace expiration pair with Expiration struct

ad6ee95

lordgamez force-pushed the MINIFICPP-1396 branch from fd2e4fa to ad6ee95 Compare January 6, 2021 11:00

adamdebreceni reviewed Jan 6, 2021

View reviewed changes

hunyadi-dev reviewed Jan 6, 2021

View reviewed changes

hunyadi-dev approved these changes Jan 6, 2021

View reviewed changes

lordgamez added 6 commits January 6, 2021 18:27

Make S3 processor onTrigger calls thread safe

e59580a

Adjust rendering of S3 processor documentation

9417f98

Use createTempDir from test utils

7b57ab6

Use verifyLogLinePresenceInPollTime instead of contains

840a645

Extract get_file_content to FileUtils

6097258

Fix linter issues

2da2360

adamdebreceni approved these changes Jan 8, 2021

View reviewed changes

arpadboda closed this in f239119 Jan 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINIFICPP-1396 Create FetchS3Object processor #970

MINIFICPP-1396 Create FetchS3Object processor #970

lordgamez commented Jan 5, 2021

arpadboda left a comment

adamdebreceni Jan 6, 2021

arpadboda Jan 6, 2021

lordgamez Jan 6, 2021

adamdebreceni Jan 6, 2021

lordgamez Jan 6, 2021

adamdebreceni Jan 6, 2021 •

edited

adamdebreceni Jan 6, 2021

lordgamez Jan 6, 2021

arpadboda Jan 6, 2021

adamdebreceni Jan 6, 2021

adamdebreceni Jan 6, 2021 •

edited

lordgamez Jan 6, 2021

hunyadi-dev Jan 6, 2021

lordgamez Jan 7, 2021

hunyadi-dev Jan 6, 2021

lordgamez Jan 7, 2021

hunyadi-dev Jan 6, 2021

lordgamez Jan 7, 2021

hunyadi-dev Jan 6, 2021

hunyadi-dev Jan 6, 2021

lordgamez Jan 7, 2021

hunyadi-dev left a comment

MINIFICPP-1396 Create FetchS3Object processor #970

MINIFICPP-1396 Create FetchS3Object processor #970

Conversation

lordgamez commented Jan 5, 2021

For all changes:

For code changes:

For documentation related changes:

Note:

arpadboda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamdebreceni Jan 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamdebreceni Jan 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hunyadi-dev left a comment

Choose a reason for hiding this comment

adamdebreceni Jan 6, 2021 •

edited

adamdebreceni Jan 6, 2021 •

edited