Dell: Add Dell EMC EcsFileIO. #3376

wang-x-xia · 2021-10-26T11:26:41Z

Use Dell EMC ECS object SDK to implement File IO APIs:

Add :iceberg-dell module and dependencies.
Create classes for FileIO:
1. EcsAppendOutputStream which uses Put and Append API to concat data bytes without local file cache.
2. EcsSeekableInputStream which uses Get-by-range API to fetch data bytes from object.
3. EcsURI, EcsFile and EcsFileIO for that basic functions.

To use the location like ecs://bucket/object_name to consistent with other vendors.

Due to Dell EMC ECS is an S3 compatible storage. The location is also compatible with schema "s3", "s3a", and "s3n".

2. Change name of some methods .

wang-x-xia · 2021-10-26T11:31:56Z

The test mock used a client-side mock that is easier for this repo.
We decide to publish an ECS Mock in the future with more internal discussion and process.

jackye1995

Thanks for separating the PR out! Here are some general comments I have:

for EcsFile, I think it's better to have separated EcsInputFile and EcsOutputFile with a base BaseEcsFile, which follows the pattern of S3 and Hadoop and is more clear to code readers.
some classes can be made package private instead of public, such as the stream classes, please make changes accordingly.
For tests, please use more expressive test names, and provide messages for each Assert. For testing errors, we prefer to use AssertHelper.assertThrows instead of @Test(expected=xxx) or try-catch block to verify error messages.
please add a newline after each control statement like if, for, try, etc.

dell/src/main/java/org/apache/iceberg/dell/EcsClientProperties.java

dell/src/main/java/org/apache/iceberg/dell/LocationUtils.java

@test

2. Move EcsAppendOutputStream and EcsSeekableInputStream to package private. 3. Fix tests with @test(expected) 4. Add new line for all control block statements. 5. Add ecs prefix and properties key in EcsClientProperties. 6. Inline LocationUtils.ECS_SCHEMA.

wang-x-xia · 2021-10-27T03:23:42Z

According to @jackye1995 's suggestion.
The EcsFile is separate to EcsInputFile and EcsOutputFile.

dell/src/main/java/org/apache/iceberg/dell/EcsClientProperties.java

dell/src/main/java/org/apache/iceberg/dell/EcsFileIO.java

dell/src/main/java/org/apache/iceberg/dell/EcsOutputFile.java

dell/src/main/java/org/apache/iceberg/dell/EcsSeekableInputStream.java

dell/src/main/java/org/apache/iceberg/dell/EcsURI.java

dell/src/main/java/org/apache/iceberg/dell/LocationUtils.java

dell/src/test/java/org/apache/iceberg/dell/EcsAppendOutputStreamTest.java

jackye1995 · 2021-11-03T16:04:30Z

dell/src/main/java/org/apache/iceberg/dell/EcsFileIO.java

+/**
+ * A {@link java.io.Externalizable} FileIO of ECS S3 object client.
+ */
+public class EcsFileIO implements FileIO, Externalizable, AutoCloseable {


I think we follow the pattern of (1) directly serialize an object like client as a function, example: https://github.com/apache/iceberg/blob/master/aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java#L42-L45. (2) if absolutely necessary, directly overwrite serialization private methods instead of using Externalize, example: https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/hadoop/SerializableConfiguration.java#L39-L48. Is it possible for you to use the same pattern here?

Took a look at the javadoc for Externalizable, it seems to be a valid usage here, so I will leave it as is for now and let other reviewers decide.

build.gradle

dell/src/test/java/org/apache/iceberg/dell/EcsFileTest.java

2. Merge LocationUtils to EcsURI. 3. Remove redundant dependencies in Gradle. 4. Some typo and style changes.

dell/src/test/java/org/apache/iceberg/dell/TestEcsSeekableInputStream.java

dell/src/test/java/org/apache/iceberg/dell/TestEcsURI.java

dell/src/test/java/org/apache/iceberg/dell/mock/TestExceptionCode.java

2. Move BaseEcsFile, EcsInputFile, EcsOutputFile to package private. 3. Separate test from single method to multiple method. Move related test rule to class-level. 4. Remove all static-imports.

jackye1995 · 2021-11-05T16:08:44Z

@rdblue I think this looks good to me overall after a few rounds of review, could you also take a look?

mechgouki · 2021-11-17T23:21:04Z

@rdblue Ryan, after several rounds's review with @jackye1995 , I think we are good to merge this PR. However this is a new feature for iceberg to support on-perm product, would you please help to take a look and give the green light ? I think the 0.13 window still open and we would like to get ready soon for our customers.

jackye1995

I added a few new comments based on the new addition of the GcsFileIO and also some discussions around aligning different vendor properties. Please let me know if you have any concern.

dell/src/main/java/org/apache/iceberg/dell/EcsClientProperties.java

jackye1995 · 2022-01-10T18:09:18Z

dell/src/main/java/org/apache/iceberg/dell/EcsFileIO.java

+/**
+ * A {@link java.io.Externalizable} FileIO of ECS S3 object client.
+ */
+public class EcsFileIO implements FileIO, Externalizable, AutoCloseable {


I think we need to directly serialize an object like client as a function instead of using Externalizable. I understand this method is likely correct, but given the fact that all the other implementations are using that approach, it's better to be consistent so that it's easier to maintain in the future.

examples:

https://github.com/apache/iceberg/blob/master/aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java#L42-L45

https://github.com/apache/iceberg/blob/master/gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSFileIO.java#L47

# Conflicts: # settings.gradle # versions.props

jackye1995 · 2022-01-12T07:21:23Z

dell/src/main/java/org/apache/iceberg/dell/DellProperties.java

+  /**
+   * S3 Access key id of Dell EMC ECS
+   */
+  public static final String ACCESS_KEY_ID = "ecs.s3.access-key-id";


should this be ECS_S3_ACCESS_KEY_ID?

and similar comments to the other variable names

jackye1995 · 2022-01-12T07:23:43Z

@rdblue any comments on this PR?

openinx

Almost looks good to me, just left few comments.

build.gradle

dell/src/main/java/org/apache/iceberg/dell/DellProperties.java

dell/src/main/java/org/apache/iceberg/dell/ecs/EcsAppendOutputStream.java

dell/src/main/java/org/apache/iceberg/dell/DellProperties.java

dell/src/main/java/org/apache/iceberg/dell/ecs/EcsURI.java

dell/src/main/java/org/apache/iceberg/dell/DellClientFactory.java

2. Remove hashcode and equals of EcsURI.

openinx

This PR looks good to me now. I will get this merged. Thanks @wang-x-xia for contribution, and thanks @jackye1995 for reviewing !

openinx · 2022-02-18T06:21:18Z

@wang-x-xia Would you also like to add a new doc page to show people how to access apache iceberg tables backed with Dell EMC ECS storage in this https://github.com/apache/iceberg-docs project ? ( Then it will display the rendered page in the apache official page).

mechgouki · 2022-02-18T06:44:09Z

@wang-x-xia Would you also like to add a new doc page to show people how to access apache iceberg tables backed with Dell EMC ECS storage in this https://github.com/apache/iceberg-docs project ? ( Then it will display the rendered page in the apache official page).

Sure, we will work on that soon.

wang-x-xia · 2022-02-18T06:45:16Z

@openinx Fine! I'll finish the doc and also the catalog implementation!

wang-x-xia added 3 commits October 25, 2021 12:21

Ecs File IO.

c033947

Add unit test with client-side mock.

e1eea69

1. Fix the copyright.

af7e750

2. Change name of some methods .

github-actions bot added the build label Oct 26, 2021

wang-x-xia mentioned this pull request Oct 26, 2021

[2806]Dell EMC ECS features required by a new ECS Catalog impl #2847

Closed

wang-x-xia mentioned this pull request Oct 26, 2021

[2806]Dell EMC ECS catalog implementation #2807

Closed

jackye1995 reviewed Oct 26, 2021

View reviewed changes

dell/src/main/java/org/apache/iceberg/dell/EcsClientProperties.java Outdated Show resolved Hide resolved

dell/src/main/java/org/apache/iceberg/dell/LocationUtils.java Outdated Show resolved Hide resolved

wang-x-xia added 2 commits October 27, 2021 11:33

Fix Copyright.

c9b785a

Fix checkStyle.

96f85a1