You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/10/11 03:06:31 UTC
[GitHub] [iceberg] xingbowu opened a new pull request #3270: Aliyun: a single bundled dependency that includes dependent JARs
xingbowu opened a new pull request #3270:
URL: https://github.com/apache/iceberg/pull/3270
Per aliyun oss SDK documentation: https://help.aliyun.com/document_detail/32009.html,
jdk9 and its plus version, dependency includes jaxb-api, activation and jaxb-runtime.
This PR to package them into one single bundled JAR
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on pull request #3270: Aliyun: A single bundled dependency that includes dependent JARs
Posted by GitBox <gi...@apache.org>.
openinx commented on pull request #3270:
URL: https://github.com/apache/iceberg/pull/3270#issuecomment-987509603
As we will introduce more and more vendor's integration modules, I think it's time to consider the strategy to handle the relationship between iceberg core/api/engines modules and vendor integration modules.
Currently, we have introduced the aws & nessie modules and we added them into the engines runtime modules by default. As more and more integration come in, the engine runtime modules will be even more fatter. The correct way is making them to be pluggable modules, for example the apache hadoop release don't include all the cloud vendor's object storage implementations into the default classpath, but it makes those modules to be hadoop's plugin implementations and if people want to enable the specific vendor implementation then they just enable it.
For our apache iceberg integration case, I think we can also make the `iceberg-aliyun` to be optional plugin modules and we build `iceberg-aliyun` module with including all dependencies jars. If people want to access the iceberg tables backed aliyun oss, then they just need to load the `iceberg-aliyun` jar. Does that make sense (also for future vendor integration ) ?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #3270: Aliyun: A single bundled dependency that includes dependent JARs
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #3270:
URL: https://github.com/apache/iceberg/pull/3270#issuecomment-987708916
> or our apache iceberg integration case, I think we can also make the iceberg-aliyun to be optional plugin modules and we build iceberg-aliyun module with including all dependencies jars. If people want to access the iceberg tables backed aliyun oss, then they just need to load the iceberg-aliyun jar
Does that mean you need to have 1 `iceberg-aliyun` jar, and another `iceberg-aliyun-runtime` jar, so the user experience become:
```
./bin/sql-client.sh -j /path/to/iceberg-aliyun-runtime.jar shell
```
I think my understanding is probably wrong... could you provide an example of the e2e integration steps?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] xingbowu closed pull request #3270: Aliyun: A single bundled dependency that includes dependent JARs
Posted by GitBox <gi...@apache.org>.
xingbowu closed pull request #3270:
URL: https://github.com/apache/iceberg/pull/3270
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #3270: Aliyun: A single bundled dependency that includes dependent JARs
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #3270:
URL: https://github.com/apache/iceberg/pull/3270#issuecomment-985771772
I read the Aliyun documentation, which says:
```
在Maven工程中使用OSS Java SDK，只需在pom.xml中加入相应依赖即可。以3.10.2版本为例，在<dependencies>中加入如下内容：
<dependency>
<groupId>com.aliyun.oss</groupId>
<artifactId>aliyun-sdk-oss</artifactId>
<version>3.10.2</version>
</dependency>
如果使用的是Java 9及以上的版本，则需要添加jaxb相关依赖。添加jaxb相关依赖示例代码如下：
<dependency>
<groupId>javax.xml.bind</groupId>
<artifactId>jaxb-api</artifactId>
<version>2.3.1</version>
</dependency>
<dependency>
<groupId>javax.activation</groupId>
<artifactId>activation</artifactId>
<version>1.1.1</version>
</dependency>
<!-- no more than 2.3.3-->
<dependency>
<groupId>org.glassfish.jaxb</groupId>
<artifactId>jaxb-runtime</artifactId>
<version>2.3.3</version>
</dependency>
```
In Iceberg the `aliyun-sdk-oss` is marked as compile only anyway, so I think this PR is not needed because to use the `aliyun` module users has to add all the related dependencies manually anyway.
Please let me know if I missed anything, otherwise I think we can close 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on a change in pull request #3270: Aliyun: A single bundled dependency that includes dependent JARs
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #3270:
URL: https://github.com/apache/iceberg/pull/3270#discussion_r762804963
##########
File path: build.gradle
##########
@@ -285,9 +285,16 @@ project(':iceberg-aliyun') {
implementation project(':iceberg-common')
compileOnly 'com.aliyun.oss:aliyun-sdk-oss'
- compileOnly 'javax.xml.bind:jaxb-api'
- compileOnly 'javax.activation:activation'
- compileOnly 'org.glassfish.jaxb:jaxb-runtime'
+ if (jdkVersion <= '8') {
+ compileOnly 'javax.xml.bind:jaxb-api'
+ compileOnly 'javax.activation:activation'
+ compileOnly 'org.glassfish.jaxb:jaxb-runtime'
+ } else {
Review comment:
For JDK8, we don't need to add those to compile classpath because the aliyun document says only greater JDK versions need this.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a change in pull request #3270: Aliyun: A single bundled dependency that includes dependent JARs
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3270:
URL: https://github.com/apache/iceberg/pull/3270#discussion_r763454941
##########
File path: build.gradle
##########
@@ -285,9 +285,16 @@ project(':iceberg-aliyun') {
implementation project(':iceberg-common')
compileOnly 'com.aliyun.oss:aliyun-sdk-oss'
Review comment:
I think service dependency jars are not added to the bundled jar to minimize bundled jar size.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #3270: Aliyun: A single bundled dependency that includes dependent JARs
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #3270:
URL: https://github.com/apache/iceberg/pull/3270#issuecomment-987325605
We only have 1 bundled jar for an engine for all JDK versions, so we have to either include all the dependencies, or no dependency at all.
I think the reason we did not include the jars for AWS was because we didn't want to inflate the bundled jar size. That's something we also need to consider here. For Aliyun, I am checking the maven repo, to include all dependencies for OSS, it also needs 3 other direct dependencies (core, ram, kms), all their dependencies, and the 3 javax jars. The runtime jar of jaxb was over 1MB. So it's not a small dependency footprint.
I have not checked if the jars conflict with any jars in the engine, that's something we also need to be careful. Plus it also creates an inconsistent experience across intergrated services, so I am a bit hesitated here. @rdblue any thoughts?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] xingbowu commented on pull request #3270: Aliyun: A single bundled dependency that includes dependent JARs
Posted by GitBox <gi...@apache.org>.
xingbowu commented on pull request #3270:
URL: https://github.com/apache/iceberg/pull/3270#issuecomment-951940315
@rdblue @kbendick Thanks for your attention on this PR. Indeed it only get some dependencies in here.
I agree with that any common dependency may cause version compatibility issues due to kinds of reasons.
and will make relocation later.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] xingbowu commented on pull request #3270: Aliyun: A single bundled dependency that includes dependent JARs
Posted by GitBox <gi...@apache.org>.
xingbowu commented on pull request #3270:
URL: https://github.com/apache/iceberg/pull/3270#issuecomment-989631070
I will close this PR if no further discussion
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on a change in pull request #3270: Aliyun: A single bundled dependency that includes dependent JARs
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #3270:
URL: https://github.com/apache/iceberg/pull/3270#discussion_r762807102
##########
File path: build.gradle
##########
@@ -285,9 +285,16 @@ project(':iceberg-aliyun') {
implementation project(':iceberg-common')
compileOnly 'com.aliyun.oss:aliyun-sdk-oss'
Review comment:
If we plan to add this into bundled `aliyun-iceberg` jar, then this line should use `implementation` rather than `compileOnly`. Because we only add classes from `implementation` jars when packaging the whole iceberg jar.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #3270: Aliyun: A single bundled dependency that includes dependent JARs
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3270:
URL: https://github.com/apache/iceberg/pull/3270#issuecomment-950395901
> Per aliyun oss SDK documentation: https://help.aliyun.com/document_detail/32009.html,
> jdk9 and its plus version, dependency includes jaxb-api, activation and jaxb-runtime.
> This PR to package them into one single bundled JAR
Given that these are recommended dependencies for using the aliyun-oss package in general (from what I understand at least), is there a chance that the versions chosen here will conflict with the versions a user chose for other needs in their project (stemming from usage of aliyun or otherwise)?
I agree with @rdblue that this seems like a case where a shaded jar with relocation of these packages seems like a good idea. If these packages are needed to make Iceberg run, then their inclusion makes sense. But it doesn’t seem in line with current practices in this repo and elsewhere for the Iceberg dependency to enforce a specific version for other components in the user’s package or to have users implicitly depending on Iceberg for packages that are needed for other parts of their application. Iceberg should really only handle the Iceberg dependencies, and then other commonly used dependencies should be relocated.
As an example, that’s why all of Iceberg’s usage of parquet goes through `org.apache.Iceberg.relocated.org.apache.parquet`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on pull request #3270: Aliyun: A single bundled dependency that includes dependent JARs
Posted by GitBox <gi...@apache.org>.
openinx commented on pull request #3270:
URL: https://github.com/apache/iceberg/pull/3270#issuecomment-986562001
@jackye1995 , We know the aws sdk provides a bundled jar to include all the dependency jars from all kinds of amazon web services, right ? Currently, the aliyun sdk don't provide that bundled jar, then if people plan to test the iceberg on oss, then they will need add the dependencies one by one ( Let's take the flink engines as an example):
```
./bin/sql-client.sh -j /path/to/<aliyun-oss-sdk>.jar \
-j /path/to/<jaxb-api>.jar \
-j /path/to/< activation>.jar \
-j /path/to/<jaxb-runtime>jar
shell
```
This exposes too much detail to the user, which makes it's harder for people to get their first quickstart works fine. So I recommend to bundle all the dependency jars into our apache iceberg released `iceberg-aliyun` jar, then people don't have to add them one by one.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #3270: Aliyun: A single bundled dependency that includes dependent JARs
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #3270:
URL: https://github.com/apache/iceberg/pull/3270#issuecomment-987536501
Trying to remember why the code was bundled in that way, it's in #1887. A shot summary of the approach used there was that:
1. use 1MB as a threshold, if the total size of the vendor's runtime module is less than 1MB, just include it
2. otherwise, we can include the iceberg integration code, but the vendor dependency is marked as provided and not bundled in the runtime jar.
But I think what @openinx suggests also works, and it's a clean user experience.
The reason we did not do that for the AWS module was that people usually have their preferred AWS SDK version that is used across their environment and upgraded together, so we don't want people to be bundled to an exact AWS SDK version that we offer. For example, AWS EMR maintains its own AWS SDK version and it's already in the java classpath, so user actually don't need to specify all those additional AWS SDK dependencies anyway, specifying it would potentially cause issue because of 2 SDK versions coexist. I don't know if this's also a concern for Aliyun.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on pull request #3270: Aliyun: A single bundled dependency that includes dependent JARs
Posted by GitBox <gi...@apache.org>.
openinx commented on pull request #3270:
URL: https://github.com/apache/iceberg/pull/3270#issuecomment-987700662
> For example, AWS EMR maintains its own AWS SDK version and it's already in the java classpath, so user actually don't need to specify all those additional AWS SDK dependencies anyway, specifying it would potentially cause issue because of 2 SDK versions coexist
The way that AWS EMR put all the common aws sdk dependencies into a common path and let all the upper layer components to refer them, I think it handle the aws sdk conflict issue correctly. In our Aliyun EMR, we don't put all of the aliyun sdk into a common path and make all the components refer it, instead we guarantee all the upper components are using the same aliyun sdk version and packaging them into their specific components classpath. Although those components are packaging the same versioned aliyun sdk and introduce the duplicated sdk jars, but they are using the same version, so in theory we can use anyone of them.
Another difference from aliyun sdk is: we aliyun don't publish the bundled aliyun sdk to aliyun users, so people have to choose their needed aliyun sdk jars and package them into a bundled jar by themself (if necessary). I think most of the other vendors choose this approach. So that's why I proposed the plugin approach.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on pull request #3270: Aliyun: A single bundled dependency that includes dependent JARs
Posted by GitBox <gi...@apache.org>.
openinx commented on pull request #3270:
URL: https://github.com/apache/iceberg/pull/3270#issuecomment-987713943
Fine, Let me create another PR to show the complete example.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3270: Aliyun: A single bundled dependency that includes dependent JARs
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3270:
URL: https://github.com/apache/iceberg/pull/3270#issuecomment-940142106
@xingbowu, This doesn't add a shaded Jar, it just updates the dependencies. Did you intend to add a shadow Jar configuration?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org