You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/04/19 15:39:19 UTC
[GitHub] [nifi-minifi-cpp] lordgamez opened a new pull request, #1306: MINIFICPP-1802 Do not make EC2 HTTP calls when AWS extension is not used
lordgamez opened a new pull request, #1306:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1306
https://issues.apache.org/jira/browse/MINIFICPP-1802
---------------------------------------------------------
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](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1306: MINIFICPP-1802 Do not make EC2 HTTP calls when AWS extension is not used
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1306:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1306#discussion_r854095069
##########
extensions/aws/processors/FetchS3Object.cpp:
##########
@@ -74,7 +74,7 @@ std::optional<aws::s3::GetObjectRequestParameters> FetchS3Object::buildFetchS3Re
const std::shared_ptr<core::ProcessContext> &context,
const std::shared_ptr<core::FlowFile> &flow_file,
const CommonProperties &common_properties) const {
- minifi::aws::s3::GetObjectRequestParameters get_object_params(common_properties.credentials, client_config_);
+ minifi::aws::s3::GetObjectRequestParameters get_object_params(common_properties.credentials, gsl::make_not_null(client_config_.get()));
Review Comment:
I would add a precondition about the validity of `client_config_` in each member that depends on it, just to explicitly document the requirements.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1306: MINIFICPP-1802 Do not make EC2 HTTP calls when AWS extension is not used
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1306:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1306#discussion_r854087624
##########
libminifi/test/aws-tests/MockS3RequestSender.h:
##########
@@ -290,5 +290,5 @@ class MockS3RequestSender : public minifi::aws::s3::S3RequestSender {
bool return_empty_result_ = false;
bool is_listing_truncated_ = false;
Aws::Auth::AWSCredentials credentials_;
- Aws::Client::ClientConfiguration client_config_;
+ Aws::Client::ClientConfiguration* client_config_;
Review Comment:
Good point, updated in 1f4cadb5a3ddac9459b2d1a3a7520f21bb36f103
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm closed pull request #1306: MINIFICPP-1802 Do not make EC2 HTTP calls when AWS extension is not used
Posted by GitBox <gi...@apache.org>.
szaszm closed pull request #1306: MINIFICPP-1802 Do not make EC2 HTTP calls when AWS extension is not used
URL: https://github.com/apache/nifi-minifi-cpp/pull/1306
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1306: MINIFICPP-1802 Do not make EC2 HTTP calls when AWS extension is not used
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1306:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1306#discussion_r854093716
##########
extensions/aws/s3/S3ClientRequestSender.cpp:
##########
@@ -31,8 +31,8 @@ namespace s3 {
std::optional<Aws::S3::Model::PutObjectResult> S3ClientRequestSender::sendPutObjectRequest(
const Aws::S3::Model::PutObjectRequest& request,
const Aws::Auth::AWSCredentials& credentials,
- const Aws::Client::ClientConfiguration& client_config) {
- Aws::S3::S3Client s3_client(credentials, client_config);
+ gsl::not_null<Aws::Client::ClientConfiguration*> client_config) {
+ Aws::S3::S3Client s3_client(credentials, *client_config);
Review Comment:
Consider using a reference instead of `gsl::not_null<Aws::Client::ClientConfiguration*>`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1306: MINIFICPP-1802 Do not make EC2 HTTP calls when AWS extension is not used
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1306:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1306#discussion_r856141519
##########
libminifi/test/aws-tests/MockS3RequestSender.h:
##########
@@ -227,7 +227,7 @@ class MockS3RequestSender : public minifi::aws::s3::S3RequestSender {
const Aws::Client::ClientConfiguration& client_config) override {
head_object_request = request;
credentials_ = credentials;
- client_config_ = client_config;
+ client_config_ = const_cast<Aws::Client::ClientConfiguration*>(&client_config);
Review Comment:
Updated in 49a58f547453a95fedfac17a992dc21838a4b714 due to the change to std::optional.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1306: MINIFICPP-1802 Do not make EC2 HTTP calls when AWS extension is not used
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on code in PR #1306:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1306#discussion_r854084015
##########
libminifi/test/aws-tests/MockS3RequestSender.h:
##########
@@ -290,5 +290,5 @@ class MockS3RequestSender : public minifi::aws::s3::S3RequestSender {
bool return_empty_result_ = false;
bool is_listing_truncated_ = false;
Aws::Auth::AWSCredentials credentials_;
- Aws::Client::ClientConfiguration client_config_;
+ Aws::Client::ClientConfiguration* client_config_;
Review Comment:
should this be initialized to `nullptr`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1306: MINIFICPP-1802 Do not make EC2 HTTP calls when AWS extension is not used
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1306:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1306#discussion_r856141144
##########
extensions/aws/processors/S3Processor.h:
##########
@@ -115,7 +115,7 @@ class S3Processor : public core::Processor {
std::shared_ptr<core::logging::Logger> logger_;
aws::s3::S3Wrapper s3_wrapper_;
- Aws::Client::ClientConfiguration client_config_;
+ std::unique_ptr<Aws::Client::ClientConfiguration> client_config_;
Review Comment:
Good point, makes the PR much simpler as well, updated in 49a58f547453a95fedfac17a992dc21838a4b714
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1306: MINIFICPP-1802 Do not make EC2 HTTP calls when AWS extension is not used
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1306:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1306#discussion_r854210630
##########
extensions/aws/processors/FetchS3Object.cpp:
##########
@@ -74,7 +74,7 @@ std::optional<aws::s3::GetObjectRequestParameters> FetchS3Object::buildFetchS3Re
const std::shared_ptr<core::ProcessContext> &context,
const std::shared_ptr<core::FlowFile> &flow_file,
const CommonProperties &common_properties) const {
- minifi::aws::s3::GetObjectRequestParameters get_object_params(common_properties.credentials, client_config_);
+ minifi::aws::s3::GetObjectRequestParameters get_object_params(common_properties.credentials, gsl::make_not_null(client_config_.get()));
Review Comment:
Added checks in b3a7f4fb48c7e1ffa069c2bf9d2bc6ad01191b33
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1306: MINIFICPP-1802 Do not make EC2 HTTP calls when AWS extension is not used
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1306:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1306#discussion_r854210956
##########
extensions/aws/s3/S3ClientRequestSender.cpp:
##########
@@ -31,8 +31,8 @@ namespace s3 {
std::optional<Aws::S3::Model::PutObjectResult> S3ClientRequestSender::sendPutObjectRequest(
const Aws::S3::Model::PutObjectRequest& request,
const Aws::Auth::AWSCredentials& credentials,
- const Aws::Client::ClientConfiguration& client_config) {
- Aws::S3::S3Client s3_client(credentials, client_config);
+ gsl::not_null<Aws::Client::ClientConfiguration*> client_config) {
+ Aws::S3::S3Client s3_client(credentials, *client_config);
Review Comment:
Changed to references in faa18039854c717de94c993fa5bc882d85d730cd
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1306: MINIFICPP-1802 Do not make EC2 HTTP calls when AWS extension is not used
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1306:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1306#discussion_r855338573
##########
extensions/aws/processors/S3Processor.h:
##########
@@ -115,7 +115,7 @@ class S3Processor : public core::Processor {
std::shared_ptr<core::logging::Logger> logger_;
aws::s3::S3Wrapper s3_wrapper_;
- Aws::Client::ClientConfiguration client_config_;
+ std::unique_ptr<Aws::Client::ClientConfiguration> client_config_;
Review Comment:
Could `std::optional` work here instead of `std::unique_ptr`? It makes late-init possible without an allocation.
##########
libminifi/test/aws-tests/MockS3RequestSender.h:
##########
@@ -227,7 +227,7 @@ class MockS3RequestSender : public minifi::aws::s3::S3RequestSender {
const Aws::Client::ClientConfiguration& client_config) override {
head_object_request = request;
credentials_ = credentials;
- client_config_ = client_config;
+ client_config_ = const_cast<Aws::Client::ClientConfiguration*>(&client_config);
Review Comment:
Maybe it would be easier to make `client_config_` `const` instead of const_casting everywhere? I only found reading from the client config in the tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org