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 2022/11/23 21:20:09 UTC
[GitHub] [pinot] xiangfu0 opened a new pull request, #9855: Upgrade adls lib, support non-cred client
xiangfu0 opened a new pull request, #9855:
URL: https://github.com/apache/pinot/pull/9855
1. upgrade azure-data-lake-store-sdk version to 2.3.10
2. upgrade azure-identity version to 1.7.1
3. use DataLakeFileSystemClient for open method as well
4. support a new auth type `NONE`, meaning no cred is added.
--
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: commits-unsubscribe@pinot.apache.org
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] [pinot] xiangfu0 commented on a diff in pull request #9855: ADLS file system upgrade
Posted by GitBox <gi...@apache.org>.
xiangfu0 commented on code in PR #9855:
URL: https://github.com/apache/pinot/pull/9855#discussion_r1031712065
##########
pinot-plugins/pinot-file-system/pinot-adls/src/main/java/org/apache/pinot/plugin/filesystem/ADLSGen2PinotFS.java:
##########
@@ -191,20 +182,22 @@ public void init(PinotConfiguration config) {
clientSecretCredentialBuilder.httpClient(builder.build());
dataLakeServiceClientBuilder.credential(clientSecretCredentialBuilder.build());
- blobServiceClientBuilder.credential(clientSecretCredentialBuilder.build());
+ break;
+ }
+ case NONE: {
+ LOGGER.info("Authenticating using anonymous access");
break;
}
default:
- throw new IllegalStateException("Expecting valid authType. One of (ACCESS_KEY, AZURE_AD, AZURE_AD_WITH_PROXY");
+ throw new IllegalArgumentException(
Review Comment:
Make sense, updated the default behavior to use `DefaultAzureCredentialBuilder`: https://learn.microsoft.com/en-us/java/api/com.azure.identity.defaultazurecredentialbuilder?view=azure-java-stable
--
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: commits-unsubscribe@pinot.apache.org
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] [pinot] xiangfu0 merged pull request #9855: ADLS file system upgrade
Posted by GitBox <gi...@apache.org>.
xiangfu0 merged PR #9855:
URL: https://github.com/apache/pinot/pull/9855
--
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: commits-unsubscribe@pinot.apache.org
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] [pinot] snleee commented on a diff in pull request #9855: ADLS file system upgrade
Posted by GitBox <gi...@apache.org>.
snleee commented on code in PR #9855:
URL: https://github.com/apache/pinot/pull/9855#discussion_r1031033488
##########
pinot-plugins/pinot-file-system/pinot-adls/src/main/java/org/apache/pinot/plugin/filesystem/ADLSGen2PinotFS.java:
##########
@@ -586,15 +579,8 @@ public boolean touch(URI uri)
@Override
public InputStream open(URI uri)
throws IOException {
- // Use Blob API since read() function from Data Lake Client currently takes "OutputStream" as an input and
- // flush bytes to an output stream. This needs to be piped back into input stream to implement this function.
- // On the other hand, Blob API directly allow you to open the input stream.
- BlobClient blobClient = _blobServiceClient.getBlobContainerClient(_fileSystemClient.getFileSystemName())
- .getBlobClient(AzurePinotFSUtil.convertUriToUrlEncodedAzureStylePath(uri));
-
- return blobClient.openInputStream();
- // Another approach is to download the file to the local disk to a temp path and return the file input stream. In
- // this case, we need to override "close()" and delete temp file.
+ return _fileSystemClient.getFileClient(AzurePinotFSUtil.convertUriToUrlEncodedAzureStylePath(uri)).openInputStream()
Review Comment:
Thanks for addressing this!
##########
pinot-plugins/pinot-file-system/pinot-adls/src/main/java/org/apache/pinot/plugin/filesystem/ADLSGen2PinotFS.java:
##########
@@ -191,20 +182,22 @@ public void init(PinotConfiguration config) {
clientSecretCredentialBuilder.httpClient(builder.build());
dataLakeServiceClientBuilder.credential(clientSecretCredentialBuilder.build());
- blobServiceClientBuilder.credential(clientSecretCredentialBuilder.build());
+ break;
+ }
+ case NONE: {
+ LOGGER.info("Authenticating using anonymous access");
break;
}
default:
- throw new IllegalStateException("Expecting valid authType. One of (ACCESS_KEY, AZURE_AD, AZURE_AD_WITH_PROXY");
+ throw new IllegalArgumentException(
Review Comment:
Should we try to initialize the client using `DefaultAzureCredential` to have a way to pass the credentials to azure client using ENV variables (or system properties)?
##########
pinot-plugins/pinot-file-system/pinot-adls/src/main/java/org/apache/pinot/plugin/filesystem/ADLSGen2PinotFS.java:
##########
@@ -191,20 +182,22 @@ public void init(PinotConfiguration config) {
clientSecretCredentialBuilder.httpClient(builder.build());
dataLakeServiceClientBuilder.credential(clientSecretCredentialBuilder.build());
- blobServiceClientBuilder.credential(clientSecretCredentialBuilder.build());
+ break;
+ }
+ case NONE: {
Review Comment:
`ANONYMOUS_ACCESS` may be a better naming for authentication type?
--
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: commits-unsubscribe@pinot.apache.org
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] [pinot] xiangfu0 commented on a diff in pull request #9855: ADLS file system upgrade
Posted by GitBox <gi...@apache.org>.
xiangfu0 commented on code in PR #9855:
URL: https://github.com/apache/pinot/pull/9855#discussion_r1031711521
##########
pinot-plugins/pinot-file-system/pinot-adls/src/main/java/org/apache/pinot/plugin/filesystem/ADLSGen2PinotFS.java:
##########
@@ -191,20 +182,22 @@ public void init(PinotConfiguration config) {
clientSecretCredentialBuilder.httpClient(builder.build());
dataLakeServiceClientBuilder.credential(clientSecretCredentialBuilder.build());
- blobServiceClientBuilder.credential(clientSecretCredentialBuilder.build());
+ break;
+ }
+ case NONE: {
Review Comment:
done
--
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: commits-unsubscribe@pinot.apache.org
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] [pinot] codecov-commenter commented on pull request #9855: ADLS file system upgrade
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9855:
URL: https://github.com/apache/pinot/pull/9855#issuecomment-1325734257
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9855?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#9855](https://codecov.io/gh/apache/pinot/pull/9855?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (175bf58) into [master](https://codecov.io/gh/apache/pinot/commit/214095425e3cd084e2cbb53dc6a52e65cf0d6e9d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2140954) will **decrease** coverage by `1.36%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #9855 +/- ##
============================================
- Coverage 70.34% 68.98% -1.37%
- Complexity 5014 5464 +450
============================================
Files 1972 1972
Lines 105692 105692
Branches 15993 15993
============================================
- Hits 74350 72908 -1442
- Misses 26137 27658 +1521
+ Partials 5205 5126 -79
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `24.71% <ø> (+0.07%)` | :arrow_up: |
| unittests1 | `67.70% <ø> (-0.02%)` | :arrow_down: |
| unittests2 | `15.77% <ø> (+0.02%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9855?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/pinot/pull/9855/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...nverttorawindex/ConvertToRawIndexTaskExecutor.java](https://codecov.io/gh/apache/pinot/pull/9855/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvY29udmVydHRvcmF3aW5kZXgvQ29udmVydFRvUmF3SW5kZXhUYXNrRXhlY3V0b3IuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...e/pinot/common/minion/MergeRollupTaskMetadata.java](https://codecov.io/gh/apache/pinot/pull/9855/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWluaW9uL01lcmdlUm9sbHVwVGFza01ldGFkYXRhLmphdmE=) | `0.00% <0.00%> (-94.74%)` | :arrow_down: |
| [...plugin/segmentuploader/SegmentUploaderDefault.java](https://codecov.io/gh/apache/pinot/pull/9855/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1zZWdtZW50LXVwbG9hZGVyL3Bpbm90LXNlZ21lbnQtdXBsb2FkZXItZGVmYXVsdC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcGx1Z2luL3NlZ21lbnR1cGxvYWRlci9TZWdtZW50VXBsb2FkZXJEZWZhdWx0LmphdmE=) | `0.00% <0.00%> (-87.10%)` | :arrow_down: |
| [.../transform/function/MapValueTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/9855/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vTWFwVmFsdWVUcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-85.30%)` | :arrow_down: |
| [...ot/common/messages/RoutingTableRebuildMessage.java](https://codecov.io/gh/apache/pinot/pull/9855/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWVzc2FnZXMvUm91dGluZ1RhYmxlUmVidWlsZE1lc3NhZ2UuamF2YQ==) | `0.00% <0.00%> (-81.82%)` | :arrow_down: |
| [...verttorawindex/ConvertToRawIndexTaskGenerator.java](https://codecov.io/gh/apache/pinot/pull/9855/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvY29udmVydHRvcmF3aW5kZXgvQ29udmVydFRvUmF3SW5kZXhUYXNrR2VuZXJhdG9yLmphdmE=) | `5.45% <0.00%> (-80.00%)` | :arrow_down: |
| [...ache/pinot/common/lineage/SegmentLineageUtils.java](https://codecov.io/gh/apache/pinot/pull/9855/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbGluZWFnZS9TZWdtZW50TGluZWFnZVV0aWxzLmphdmE=) | `22.22% <0.00%> (-77.78%)` | :arrow_down: |
| [...ore/startree/executor/StarTreeGroupByExecutor.java](https://codecov.io/gh/apache/pinot/pull/9855/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9leGVjdXRvci9TdGFyVHJlZUdyb3VwQnlFeGVjdXRvci5qYXZh) | `0.00% <0.00%> (-77.78%)` | :arrow_down: |
| [...che/pinot/core/util/SegmentProcessorAvroUtils.java](https://codecov.io/gh/apache/pinot/pull/9855/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL1NlZ21lbnRQcm9jZXNzb3JBdnJvVXRpbHMuamF2YQ==) | `0.00% <0.00%> (-76.09%)` | :arrow_down: |
| ... and [141 more](https://codecov.io/gh/apache/pinot/pull/9855/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: commits-unsubscribe@pinot.apache.org
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] [pinot] snleee commented on a diff in pull request #9855: ADLS file system upgrade
Posted by GitBox <gi...@apache.org>.
snleee commented on code in PR #9855:
URL: https://github.com/apache/pinot/pull/9855#discussion_r1031033274
##########
pinot-plugins/pinot-file-system/pinot-adls/src/main/java/org/apache/pinot/plugin/filesystem/ADLSGen2PinotFS.java:
##########
@@ -191,20 +182,22 @@ public void init(PinotConfiguration config) {
clientSecretCredentialBuilder.httpClient(builder.build());
dataLakeServiceClientBuilder.credential(clientSecretCredentialBuilder.build());
- blobServiceClientBuilder.credential(clientSecretCredentialBuilder.build());
+ break;
+ }
+ case NONE: {
+ LOGGER.info("Authenticating using anonymous access");
break;
}
default:
- throw new IllegalStateException("Expecting valid authType. One of (ACCESS_KEY, AZURE_AD, AZURE_AD_WITH_PROXY");
+ throw new IllegalArgumentException(
Review Comment:
Should we try to initialize the client using `DefaultAzureCredential` to have a way to pass the credentials to azure client using ENV variables (or system properties)? Or, that is not scope of this PR?
##########
pinot-plugins/pinot-file-system/pinot-adls/src/main/java/org/apache/pinot/plugin/filesystem/ADLSGen2PinotFS.java:
##########
@@ -191,20 +182,22 @@ public void init(PinotConfiguration config) {
clientSecretCredentialBuilder.httpClient(builder.build());
dataLakeServiceClientBuilder.credential(clientSecretCredentialBuilder.build());
- blobServiceClientBuilder.credential(clientSecretCredentialBuilder.build());
+ break;
+ }
+ case NONE: {
+ LOGGER.info("Authenticating using anonymous access");
break;
}
default:
- throw new IllegalStateException("Expecting valid authType. One of (ACCESS_KEY, AZURE_AD, AZURE_AD_WITH_PROXY");
+ throw new IllegalArgumentException(
Review Comment:
Should we try to initialize the client using `DefaultAzureCredential` to have a way to pass the credentials to azure client using ENV variables (or system properties)? Or, that is not the scope of this PR?
--
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: commits-unsubscribe@pinot.apache.org
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] [pinot] xiangfu0 commented on pull request #9855: ADLS file system upgrade
Posted by GitBox <gi...@apache.org>.
xiangfu0 commented on PR #9855:
URL: https://github.com/apache/pinot/pull/9855#issuecomment-1326686299
@jackjlli please also check, there is a behavior change for handling the default scenario for the AzurePinotFS. Since it's on the failover code path, I won't mark it as backward incompatible.
--
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: commits-unsubscribe@pinot.apache.org
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