Add Azure Data Lake Gen2 connector for PinotFS #5116

snleee · 2020-03-05T09:58:05Z

Testing have been done by attaching ADLS Gen2 to the local deployment.
move() is implemented by copy & delete because of azure sdk issue with rename() API.
[BUG] srcUri is decoded and not encoded back for rename API (ADLS Gen2) Azure/azure-sdk-for-java#8761

mayankshriv

Does it make sense to add tests using Mockito? If not, we can add tests for utility functions such as convertAzureStylePathToUriStylePath.

...ile-system/pinot-adls/src/main/java/org/apache/pinot/plugin/filesystem/AzureGen2PinotFS.java

pom.xml

codecov-io · 2020-03-10T07:33:01Z

Codecov Report

Merging #5116 into master will decrease coverage by 0.76%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##             master    #5116      +/-   ##
============================================
- Coverage     58.32%   57.55%   -0.77%     
  Complexity       12       12              
============================================
  Files          1209     1184      -25     
  Lines         64541    62424    -2117     
  Branches       9484     9143     -341     
============================================
- Hits          37643    35929    -1714     
+ Misses        24143    23847     -296     
+ Partials       2755     2648     -107

Impacted Files	Coverage Δ	Complexity Δ
.../java/org/apache/pinot/spi/filesystem/PinotFS.java	`72.22% <66.66%> (-6.35%)`	`0 <0> (ø)`
...ation/function/AggregationFunctionVisitorBase.java	`0% <0%> (-96%)`	`0% <0%> (ø)`
...ommon/lineage/SegmentMergeLineageAccessHelper.java	`0% <0%> (-80%)`	`0% <0%> (ø)`
...n/java/org/apache/pinot/common/utils/LLCUtils.java	`0% <0%> (-75%)`	`0% <0%> (ø)`
.../org/apache/pinot/common/config/RoutingConfig.java	`0% <0%> (-70%)`	`0% <0%> (ø)`
...org/apache/pinot/common/metrics/BrokerMetrics.java	`44.44% <0%> (-44.45%)`	`0% <0%> (ø)`
.../org/apache/pinot/client/PinotClientException.java	`33.33% <0%> (-33.34%)`	`0% <0%> (ø)`
...che/pinot/core/startree/v2/StarTreeV2Metadata.java	`62.5% <0%> (-29.17%)`	`0% <0%> (ø)`
...a/manager/realtime/RealtimeSegmentDataManager.java	`50% <0%> (-25%)`	`0% <0%> (ø)`
...rg/apache/pinot/core/transport/ServerInstance.java	`53.57% <0%> (-21.43%)`	`0% <0%> (ø)`
... and 239 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5c8398...67feb10. Read the comment docs.

snleee · 2020-03-12T01:30:58Z

md5 computation time benchmark (on macbook with ssd):

(md5 compute/ total time) <- total time = IO(read contents from file) + md5 hash computation
1 mb: 17 / 20 ms
10 mb: 54 / 62 ms
100 mb: 402 / 462 ms
1000 mb: 2998 / 3497 ms

So, computing md5 hash on 1GB file adds about 3 seconds. Since this adds non-trivial overheads (depending on the use case), I updated the code to make md5 check configurable.

1. Testing have been done by attaching ADLS Gen2 to the local deployment. 2. move() is implemented by copy & delete because of azure sdk issue with rename() API. Azure/azure-sdk-for-java#8761

mcvsubbu · 2020-03-12T17:50:21Z

...ile-system/pinot-adls/src/main/java/org/apache/pinot/plugin/filesystem/AzureGen2PinotFS.java

+    _blobServiceClient =
+        new BlobServiceClientBuilder().credential(sharedKeyCredential).endpoint(blobServiceEndpointUrl).buildClient();
+    _fileSystemClient = serviceClient.getFileSystemClient(fileSystemName);
+    LOGGER.error("AzureGen2PinotFS is initialized (accountName={}, fileSystemName={}, dfsServiceEndpointUrl={}, "


why is this an error log?

good catch. I was debugging and forgot to turn it back.

jackjlli · 2020-03-12T19:44:46Z

...ile-system/pinot-adls/src/main/java/org/apache/pinot/plugin/filesystem/AzureGen2PinotFS.java

+      if (e.getStatusCode() == ALREADY_EXISTS_STATUS_CODE && e.getErrorCode().equals(PATH_ALREADY_EXISTS_ERROR_CODE)) {
+        return true;
+      }
+      LOGGER.error("Exception thrown while calling mkdir (uri = {})", uri, e);


It'd be good to print the error status code here.

I'm including e, which is exception object to the log. This should include the status code information as part of the exception stack. Do you think it's better to add status code explicitly along with uri?

Not sure whether the status code is included in the exception, but it'd be good to show it in the log. :)

jackjlli · 2020-03-12T19:48:17Z

...ile-system/pinot-adls/src/main/java/org/apache/pinot/plugin/filesystem/AzureGen2PinotFS.java

+            mkdir(newDst);
+          } else {
+            // If src is a file, we need to copy.
+            copySucceeded |= copySrcToDst(currentSrc, newDst);


What if part of the files failed? It will still return true, right?

good catch :) I updated. I also made the change to GcsPinotFS, which had the similar issue.

jackjlli · 2020-03-12T19:55:12Z

pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFS.java

-      URI parentUri = Paths.get(dstUri).getParent().toUri();
-      mkdir(parentUri);
+      try {
+        Path parentPath = Paths.get(dstUri.getPath()).getParent();


Can you add a test for testing the case when the scheme doesn't match then throw the exception?

added a test to cover this case to LocalPinotFSTest

jackjlli · 2020-03-12T19:57:02Z

...ile-system/pinot-adls/src/main/java/org/apache/pinot/plugin/filesystem/AzureGen2PinotFS.java

+/**
+ * Azure Data Lake Storage Gen2 implementation for the PinotFS interface.
+ */
+public class AzureGen2PinotFS extends PinotFS {


Do we have any test for this class? We've noticed the code coverage is only 50% though.

I don't see a good way to test. I've been testing this by hooking this up to the live ADLS Gen2. One way is to mock every single Azure SDK API that i'm calling using Mockhito but this doesn't really check much.

Another potential approach is to create the integration test by incorporating Azurite https://github.com/Azure/Azurite, which is Azure storage service emulator. But, this doesn't support Azure Datalake Gen2.

By the way, I did verify all the functions by hooking up the live Data Lake Gen2.

mayankshriv

Please address comments from @jackjlli before merging.

jackjlli

Thanks for addressing the comments! In terms of the tests, it'd still be good to have it. Even if the code works fine today, somebody else may change it in the future. And it'd be hard for another guy to test his change without following the tests you've done. You can add them later or just add a todo there. You decide. Thanks!

snleee · 2020-03-17T00:13:51Z

@jackjlli Thanks for the comment. I added TODO comment on the test. I will address it with the separate PR.

snleee requested review from jackjlli and mayankshriv March 5, 2020 09:58

mayankshriv reviewed Mar 5, 2020

View reviewed changes

jackjlli reviewed Mar 5, 2020

View reviewed changes

snleee force-pushed the adls-gen2 branch 3 times, most recently from 6112a99 to 4d2a8ff Compare March 10, 2020 03:34

snleee force-pushed the adls-gen2 branch from 4d2a8ff to 23a6b2d Compare March 11, 2020 02:55

Add Azure Data Lake Gen2 connector for PinotFS

67feb10

1. Testing have been done by attaching ADLS Gen2 to the local deployment. 2. move() is implemented by copy & delete because of azure sdk issue with rename() API. Azure/azure-sdk-for-java#8761

snleee force-pushed the adls-gen2 branch from 23a6b2d to 67feb10 Compare March 12, 2020 01:36

mcvsubbu reviewed Mar 12, 2020

View reviewed changes

jackjlli reviewed Mar 12, 2020

View reviewed changes

mayankshriv approved these changes Mar 13, 2020

View reviewed changes

jackjlli approved these changes Mar 16, 2020

View reviewed changes

Addressing comments

a9ec33c

snleee force-pushed the adls-gen2 branch from ef34445 to a9ec33c Compare March 17, 2020 00:13

snleee merged commit c81a656 into apache:master Mar 17, 2020

snleee deleted the adls-gen2 branch March 17, 2020 01:32

snleee mentioned this pull request Apr 6, 2020

[FEATURE REQ] Add parallelism config for uploading/downloading files on ADLS Gen2 Java SDK Azure/azure-sdk-for-java#9817

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Azure Data Lake Gen2 connector for PinotFS #5116

Add Azure Data Lake Gen2 connector for PinotFS #5116

snleee commented Mar 5, 2020

mayankshriv left a comment

codecov-io commented Mar 10, 2020 •

edited

Loading

snleee commented Mar 12, 2020

mcvsubbu Mar 12, 2020

snleee Mar 12, 2020

jackjlli Mar 12, 2020

snleee Mar 13, 2020 •

edited

Loading

jackjlli Mar 13, 2020

jackjlli Mar 12, 2020

snleee Mar 13, 2020

jackjlli Mar 12, 2020

snleee Mar 13, 2020

jackjlli Mar 12, 2020

snleee Mar 13, 2020 •

edited

Loading

mayankshriv left a comment

jackjlli left a comment

snleee commented Mar 17, 2020

Add Azure Data Lake Gen2 connector for PinotFS #5116

Add Azure Data Lake Gen2 connector for PinotFS #5116

Conversation

snleee commented Mar 5, 2020

mayankshriv left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 10, 2020 • edited Loading

Codecov Report

snleee commented Mar 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snleee Mar 13, 2020 • edited Loading

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

snleee Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

mayankshriv left a comment

Choose a reason for hiding this comment

jackjlli left a comment

Choose a reason for hiding this comment

snleee commented Mar 17, 2020

codecov-io commented Mar 10, 2020 •

edited

Loading

snleee Mar 13, 2020 •

edited

Loading

snleee Mar 13, 2020 •

edited

Loading