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

wang-x-xia · 2021-07-21T14:02:06Z

It's the first part of #2807 .

Dell EMC ECS is an Object Storage which delivers rich S3-compatibility.

This PR is to add new interfaces that provide several features from the Dell EMC ECS.

The main interface is EcsClient, it provides methods like:

To get object info.
To read and write object with Java InputStream/OutputStream that for data files.
To read, replace, write and copy object that for metadata files.
To list objects that for fetch concepts such as namespaces and databases.

And there are 2 additional interfaces which provide helper method of client:

PropertiesSerDes is used to convert Map<String, String> to file content. I add a default version value to avoid compatible problems in future.
ObjectKeys is used to convert name of Iceberg resources to object key.

The EcsClientImpl provide the impl that use Amazon AWS S3 Java SDK (V1) to connect with the ECS.
The Amazon AWS S3 Java SDK (V2), used in the :iceberg-aws module, doesn't provide extension.

The MemoryEcsClient provide the impl that mock all operations of EcsClient in memory. I will use in future tests.

2. Create EcsClient to define the related methods used in Iceberg.

2. remove EcsClient#getProperties.

mechgouki · 2021-08-13T02:57:58Z

@kbendick @rdblue
We would like to hear the suggestion whether we should wait for this the first new PR(1 of 3) pass the review and then start to post the rest ?
Or
Should we just go ahead to post the rest 2 PRs now?

@openinx

rdblue · 2021-09-02T16:12:34Z

build.gradle

    compile(project(':iceberg-nessie')) {
      exclude group: 'com.google.code.findbugs', module: 'jsr305'
    }
+    compile(project(':iceberg-dell'))


Please remove additions to the runtime Jars until later. We add new integrations to the runtime Jars after they are finished, and we also look at the dependencies that are added to the runtime Jar as a result. We need to be careful with runtime Jar licensing.

I don't think that we want to start worrying about updating LICENSE files and looking at sizes just yet.

rdblue · 2021-09-02T16:13:56Z

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

+   *
+   * @return utils class
+   */
+  default PropertiesSerDes getPropertiesSerDes() {


The Iceberg community doesn't use get in method names unless something needs to conform to Java Bean expectations. The reason is that get is almost never valuable: either there is a more specific verb that should be used instead, or it is basically useless and should be omitted.

rdblue · 2021-09-02T16:15:56Z

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

+import java.util.Objects;
+
+/**
+ * a immutable record class of object key


Minor: Please use sentence case in Javadoc. Sentences should start with a capital letter and end with punctuation. Also, it should be "an" when followed by a noun that begins with a vowel.

rdblue · 2021-09-02T16:16:37Z

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

+ */
+public class ObjectKey {
+  /**
+   * bucket


Please omit useless comments. Repeating the field name is not helpful.

rdblue · 2021-09-02T16:18:23Z

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

+   */
+  private final String key;
+
+  public ObjectKey(String bucket, String key) {


Something is not right here. The class is called ObjectKey, but it is created by passing a String called key. Is the string the key, or is it part of the key? Maybe this should be similar to S3, where the bucket/string combination is called a URI, S3URI.

In object storage, the object key contains both a bucket name and a key.
And in my design, the user can provide different delimiters for object storage.
For example, the user can create an object which bucket is a and key is folder2021-folder09-obj1 with delimiter -.
It's hard to format it as a string, so I move this to the ObjectKeys interface.

And I need some time to check whether using emc://bucket/key as a general string.

rdblue · 2021-09-02T16:18:49Z

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

+  }
+
+  @Override
+  public String toString() {


Does this not form a location, like emc://bucket/path?

rdblue · 2021-09-02T16:35:29Z

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

+/**
+ * use ECS append api to write data
+ */
+public class EcsAppendOutputStream extends OutputStream {


Why doesn't this implement PositionOutputStream?

The PositionOutputStream is just a simple extension of OutputStream. To implement it in this class is a little wired in OOP. I'll create an adapter class in the next PR.

rdblue · 2021-09-02T16:35:49Z

dell/src/main/java/org/apache/iceberg/dell/impl/ContentAndETagImpl.java

+
+  public ContentAndETagImpl(ObjectHeadInfo headInfo, byte[] content) {
+    this.headInfo = headInfo;
+    this.content = content;


What is content here? Is it an etag?

Content is all bytes data of an object. It's for properties objects.

rdblue · 2021-09-02T16:37:21Z

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

+   * <p>
+   * this properties is only a placeholder and may use in future.
+   */
+  String CURRENT_VERSION = "0";


What is the value of this if it isn't serialized into the blob?

No, it's in object user metadata. We wish to use it if the properties format needs to change.

rdblue · 2021-09-02T16:40:19Z

@wang-x-xia, if EMC is S3 compatible and uses the v1 S3 API, why not make this an implementation of FileIO that uses the older S3 API? Can you help me understand why this is specific to EMC?

wang-x-xia · 2021-09-03T03:05:32Z

@wang-x-xia, if EMC is S3 compatible and uses the v1 S3 API, why not make this an implementation of FileIO that uses the older S3 API? Can you help me understand why this is specific to EMC?

ECS can append data to object. So the complex data cache of the S3 FileIO impl can be omit.

The ECSClient interface is for that if Amazon deprecated their V1 SDK, it's easily to create a new implementation of this interface.

Actually, the V2 SDK is same with V1 SDK. But just V1 SDK allow to custom HTTP headers that can help ECS and other object services to provide some extension feature.
If Iceberg need a V1-SDKed FileIO, I can help to implement one. The object writes will be same with current AWS implementation with a local data cache.

ECS also have its own SDK, but the license may cause some problems. Finally I used the S3 V1 SDK.

mechgouki · 2021-09-03T05:57:07Z

@wang-x-xia, if EMC is S3 compatible and uses the v1 S3 API, why not make this an implementation of FileIO that uses the older S3 API? Can you help me understand why this is specific to EMC?

The short answer is the V2 SDK added limitation for extension, while some extension APIs will benefit Iceberg to have better performance run on our product.
As Xia said, we could switch to use DellEMC Client which also open source https://github.com/EMCECS/ecs-object-client-java
But we might need additional check on legal as the license is BSD.

2. Remove most redundant methods in EcsClient.

mechgouki · 2021-09-06T06:42:07Z

@wang-x-xia, if EMC is S3 compatible and uses the v1 S3 API, why not make this an implementation of FileIO that uses the older S3 API? Can you help me understand why this is specific to EMC?

Discussed internally, we plan to switch to DellEMC ECS Client lib here as it is fully open source and we are supporting/will support it

wang-x-xia · 2021-09-06T12:49:36Z

@rdblue Here is a problem with using our own SDK.
I noticed that https://github.com/EMCECS/ecs-object-client-java uses a BSD-3 license.
If I use this, what more actions should I take?

rdblue · 2021-09-09T23:42:37Z

@wang-x-xia, the 3-clause BSD license is compatible with the Apache License: https://www.apache.org/legal/resolved.html

It is listed as Category A. So it should be fine.

What is the rationale for using a library specific to EMC rather than the S3 v1 API? It seems like the S3 v1 API is probably going to be useful to more people. Are there EMC features that we can only get using the EMC client? You mentioned append support, but all of the formats that Iceberg uses are immutable. So we never need to re-open a file and append more data.

mechgouki · 2021-09-10T00:21:59Z

@wang-x-xia, the 3-clause BSD license is compatible with the Apache License: https://www.apache.org/legal/resolved.html

It is listed as Category A. So it should be fine.

What is the rationale for using a library specific to EMC rather than the S3 v1 API? It seems like the S3 v1 API is probably going to be useful to more people. Are there EMC features that we can only get using the EMC client? You mentioned append support, but all of the formats that Iceberg uses are immutable. So we never need to re-open a file and append more data.

Basically we have 2 areas which Dell EMC features could benefit :
(1)Append operation in additional of MPU, if the client has less local cache for large object( like edge), Dell EMC object service could help here.
(2)Atomic rename. We have If-Match and If-None-Match semantic as we support strong consistency model (within one site) from the very beginning.

And talked internally, for better support the customer, if we use Dell EMC features, we'd better use our own client with BSD License. AWS S3 V1 could be workaround here, but V2 can not be an option .

Looking forward for your suggestion here

rdblue · 2021-09-10T15:30:04Z

@mechgouki, that sounds like reasonable motivation to go with the EMC client to me. What are the dependencies of the EMC artifact?

mechgouki · 2021-09-13T09:11:15Z

@mechgouki, that sounds like reasonable motivation to go with the EMC client to me. What are the dependencies of the EMC artifact?

@rdblue
We listed all the dependencies at https://github.com/EMCECS/ecs-object-client-java/tree/master/3rd-party-licenses

wang-x-xia · 2021-10-26T11:29:00Z

Using the https://github.com/EMCECS/ecs-object-client-java, the PR is redundant to create an abstraction for ECS APIs.

The following File IO PR is #3376.
And the parent PR of ECS Catalog with related discussion is #2807.

You can check these PRs for more details.

1. Add dell module.

99f2d03

2. Create EcsClient to define the related methods used in Iceberg.

github-actions bot added the build label Jul 21, 2021

1. remove mirror repo changes.

6a89864

2. remove EcsClient#getProperties.

rdblue reviewed Sep 2, 2021

View reviewed changes

1. Review all comments.

f3bb9e0

2. Remove most redundant methods in EcsClient.

Remove all dependencies to :iceberg-dell.

596b76f

wang-x-xia closed this Oct 26, 2021

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

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

Uh oh!

Conversation

wang-x-xia commented Jul 21, 2021

Uh oh!

mechgouki commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wang-x-xia Sep 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue commented Sep 2, 2021

Uh oh!

wang-x-xia commented Sep 3, 2021

Uh oh!

mechgouki commented Sep 3, 2021

Uh oh!

mechgouki commented Sep 6, 2021

Uh oh!

wang-x-xia commented Sep 6, 2021

Uh oh!

rdblue commented Sep 9, 2021

Uh oh!

mechgouki commented Sep 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rdblue commented Sep 10, 2021

Uh oh!

mechgouki commented Sep 13, 2021

Uh oh!

wang-x-xia commented Oct 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mechgouki commented Aug 13, 2021 •

edited

Loading

wang-x-xia Sep 5, 2021 •

edited

Loading

mechgouki commented Sep 10, 2021 •

edited

Loading