You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/10/04 20:57:27 UTC
[GitHub] [incubator-pinot] danbosnichill opened a new pull request #6104: Add a property to set the s3 endpoint
danbosnichill opened a new pull request #6104:
URL: https://github.com/apache/incubator-pinot/pull/6104
## Description
Allow the S3PinotFS endpoint to be set by a property. This will allow me to configure Pinot to write to MinIO (s3 compatible interface) while running locally on my Minikube.
## Upgrade Notes
No upgrade issues. This is a new property and the default does not change the existing behavior.
Does this PR otherwise need attention when creating release notes? Things to consider:
- New configuration options
* [X] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
Added a new property to configure S3PinotFS's endpoint. This can be used to use a local S3-compatible interface.
## Documentation
If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
We can update this section after this is merged. I'm not sure if I have permission to do this.
https://docs.pinot.apache.org/basics/data-import/pinot-file-system/amazon-s3
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] fx19880617 merged pull request #6104: Add a property to set the s3 endpoint
Posted by GitBox <gi...@apache.org>.
fx19880617 merged pull request #6104:
URL: https://github.com/apache/incubator-pinot/pull/6104
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] danbosnichill commented on a change in pull request #6104: Add a property to set the s3 endpoint
Posted by GitBox <gi...@apache.org>.
danbosnichill commented on a change in pull request #6104:
URL: https://github.com/apache/incubator-pinot/pull/6104#discussion_r499322765
##########
File path: pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java
##########
@@ -93,7 +95,16 @@ public void init(PinotConfiguration config) {
awsCredentialsProvider = DefaultCredentialsProvider.create();
}
- _s3Client = S3Client.builder().region(Region.of(region)).credentialsProvider(awsCredentialsProvider).build();
+ S3ClientBuilder s3ClientBuilder = S3Client.builder().region(Region.of(region)).credentialsProvider(awsCredentialsProvider);
Review comment:
I don't think MinIO cares about region. It doesn't in the other code I'm using.
MinIO can be set up with a couple auth types. I use accessKey and secretKey with MinIO locally.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] fx19880617 commented on pull request #6104: Add a property to set the s3 endpoint
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on pull request #6104:
URL: https://github.com/apache/incubator-pinot/pull/6104#issuecomment-703319756
Overall LGTM.
Could you add more details of how to config this connector to MinIO with a sample configs into the PR description, thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #6104: Add a property to set the s3 endpoint
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6104:
URL: https://github.com/apache/incubator-pinot/pull/6104#discussion_r499382289
##########
File path: pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java
##########
@@ -93,7 +95,16 @@ public void init(PinotConfiguration config) {
awsCredentialsProvider = DefaultCredentialsProvider.create();
}
- _s3Client = S3Client.builder().region(Region.of(region)).credentialsProvider(awsCredentialsProvider).build();
+ S3ClientBuilder s3ClientBuilder = S3Client.builder().region(Region.of(region)).credentialsProvider(awsCredentialsProvider);
Review comment:
I think your code to not touch this should be ok, we can clean this up later.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #6104: Add a property to set the s3 endpoint
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6104:
URL: https://github.com/apache/incubator-pinot/pull/6104#discussion_r499291221
##########
File path: pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java
##########
@@ -93,7 +95,16 @@ public void init(PinotConfiguration config) {
awsCredentialsProvider = DefaultCredentialsProvider.create();
}
- _s3Client = S3Client.builder().region(Region.of(region)).credentialsProvider(awsCredentialsProvider).build();
+ S3ClientBuilder s3ClientBuilder = S3Client.builder().region(Region.of(region)).credentialsProvider(awsCredentialsProvider);
Review comment:
Just curious, does MinIO take region/awsCredentialProvider as parameters?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] danbosnichill commented on a change in pull request #6104: Add a property to set the s3 endpoint
Posted by GitBox <gi...@apache.org>.
danbosnichill commented on a change in pull request #6104:
URL: https://github.com/apache/incubator-pinot/pull/6104#discussion_r499323328
##########
File path: pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java
##########
@@ -93,7 +95,16 @@ public void init(PinotConfiguration config) {
awsCredentialsProvider = DefaultCredentialsProvider.create();
}
- _s3Client = S3Client.builder().region(Region.of(region)).credentialsProvider(awsCredentialsProvider).build();
+ S3ClientBuilder s3ClientBuilder = S3Client.builder().region(Region.of(region)).credentialsProvider(awsCredentialsProvider);
Review comment:
It might make sense then to make region optional. I wasn't able to test this locally (given I can't build Pinot). I think keeping it required is a safer diff.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] danbosnichill commented on pull request #6104: Add a property to set the s3 endpoint
Posted by GitBox <gi...@apache.org>.
danbosnichill commented on pull request #6104:
URL: https://github.com/apache/incubator-pinot/pull/6104#issuecomment-703368633
Updated.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org