Aliyun: Add OSSFileIO

[xingbowu]

No description provided.

[rdblue]

I don't think this method name is clear. How about newClient?

[xingbowu]

sounds good. I will make it.

[rdblue]

We generally try to avoid nesting in places that have a small set of specific properties. Is it possible to make these shorter and more standard?

[openinx]

As we will introduce the aliyun catalog services named DLF in future, the DLF services also has its endpoint. we need to prefix oss. and dlf. to distingush it's an oss endpoint or dlf endpoint. That's why we use oss.endpoint here.

[rdblue]

Looks good to me. Since I don't see an approval from @openinx, I'll wait and let him merge this when he is ready.

[jackye1995]

would it be better to make this the same as https://github.com/apache/iceberg/blob/master/aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java#L145?

[rdblue]

It would be nice if they were the same, but I like that this conforms to the -impl convention that we use for dynamically loaded classes elsewhere.

[xingbowu]

I can make another PR to line up the behavior of aws part as "client.factory-impl"

[jackye1995]

Cool, that works for me as well

[jackye1995]

When we were writing AwsClientFactory we had AwsClientFactories to vend a default client factory implementation, and that was because it seems like Iceberg does not expose the default implementation as a public class for other classes like LocationProvider, etc. I wonder if we should follow the same pattern here.

https://github.com/apache/iceberg/blob/master/aws/src/main/java/org/apache/iceberg/aws/AwsClientFactories.java

[openinx]

Yes, I agree. As we will also introduce many different factories for aliyun clients, it's good to have a proxy to delegate to create the factories.

[openinx]

It's better to name it as ossClient because I think we will introduce another aliyun client, such as dlf client etc.

[rdblue]

How aboutnewOSSClient then?

[openinx]

> Task :iceberg-arrow:compileScala NO-SOURCE
> Task :iceberg-arrow:classes
> Task :iceberg-arrow:jar
> Task :iceberg-arrow:sourceJar
Error: eckstyle] [ERROR] /home/runner/work/iceberg/iceberg/aliyun/src/test/java/org/apache/iceberg/aliyun/TestAliyunClientFactories.java:24:1: Extra separation in import group before 'java.util.HashMap' [ImportOrder]
Error: eckstyle] [ERROR] /home/runner/work/iceberg/iceberg/aliyun/src/test/java/org/apache/iceberg/aliyun/TestAliyunClientFactories.java:24:8: Unused import - java.util.HashMap. [UnusedImports]
Error: eckstyle] [ERROR] /home/runner/work/iceberg/iceberg/aliyun/src/test/java/org/apache/iceberg/aliyun/TestAliyunClientFactories.java:52:5: 'CTOR_DEF' should be separated from previous line. [EmptyLineSeparator]

> Task :iceberg-aliyun:checkstyleTest

> Task :iceberg-aliyun:checkstyleTest FAILED
Note: Some input files use or override a deprecated API.

Note: Recompile with -Xlint:deprecation for details.
> Task :iceberg-core:compileTestJava
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

131 actionable tasks: 131 executed
FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':iceberg-aliyun:checkstyleTest'.
> Checkstyle rule violations were found. See the report at: file:///home/runner/work/iceberg/iceberg/aliyun/build/reports/checkstyle/test.html
  Checkstyle files with violations: 1
  Checkstyle violations by severity: [error:3]

@xingbowu , Could you help to fix the checkstyles ?

[jackye1995]

Looks good to me, thanks for addressing the comments!

[rdblue]

Can you make this newOSSClient? We typically prefix factory method names with new.

[xingbowu]

Rework and follow up this guide

[openinx]

LGTM now ! Let's get this merged, Thanks @xingbowu for contribution and thanks @jackye1995 and @rdblue for the careful reviewing !