HADOOP-19236. Incorporate VolcanoEngine Cloud TOS File System Implementation. Part 1: add core code and docs. #7294
+14,074
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@wojiaodoubao If this part of the code is to be merged, it must ensure compatibility with JDK 17, as we are currently working on the upgrade. |
|
@slfan1989 thanks for your kindly reminder. Let me test it with JDK17. |
|
hi @slfan1989 , I take a look at HADOOP-15984, nice work! If I understand right, the blocker is upgrading jersey. The hadoop-tos dependencies include jersey through hadoop-common and hadoop-yarn-common, both provided, no direct or indirect use, so I think it's safe to merge. I notice the HADOOP-15984 is going to be merged within 2 days. It is a real hard work, we don't want any accidents. So I will |
|
💔 -1 overall
This message was automatically generated. |
pan3793
reviewed
Jan 20, 2025
| <include>com.volcengine:*</include> | ||
| <include>com.squareup.okhttp3:okhttp</include> | ||
| <include>com.squareup.okio:okio-jvm</include> | ||
| <include>org.jetbrains.kotlin:kotlin-stdlib*</include> |
There was a problem hiding this comment.
is it safe to relocate a language runtime?
I know that OKHttp was a popular HTTP client in the Java world, but since 4.x, it switched from Java to Kotlin, that means every downstream project that used OKHttp client needs to pull a new JVM language runtime, this do sounds crazy for a HTTP client!
There was a problem hiding this comment.
Hi @pan3793 , thanks your comments. The intention of shade is to ensure the tos sdk doesn't conflict with any user's classes, so the hadoop-tos could be simple and safe to use.
Here I shaded tos sdk, okhttp, okio and kotlin-stdlib, the final shaded jar is 6,226,348 bytes. So if the downstream project uses okhttp too, the classes will doubled to 12MB+. The default meta space size is 21MB, in the worst case, 12MB+ are all loaded and occupy 57% meta space, which is a problem(may cause long gc).
The calculation above is not accurate, since the classes in file is smaller than classes in JVM, and classes won't be loaded until used. But its enough to expose the problem.
Your concern is reasonable. We can leave the potential conflicts for downstream projects. They can shade hadoop-tos and it's dependencies if necessary.
There was a problem hiding this comment.
Size and class increase does not look like a big deal, concerns are relocation itself.
Class relocation is not a silver bullet, for example, it may break reflection invocations, and does not work with JNI, relocating a JVM language runtime library sounds like risky stuff.
AFAIK, the hadoop-client-runtime was not fully tested in the Hadoop project, and several issues caused by relocation were identified after Spark adoption. I think the hadoop-tos-shade stays in the same situation
How hard is it for TOS to switch to another HTTP client? i.e. Apache HTTP Client, a pure Java HTTP client, and is easy to relocate.
There was a problem hiding this comment.
an alternative: publish ve-tos-java-sdk-shaded, and downstream projects like Hadoop simply consume the single jar, this will address the test coverage concern I mentioned above and save a lot of users for handling class conflict.
I glanced TOS repo README and docs, seems OKHTTP/OKIO is not leaked to public API, so either switching to another HTTP client or creating a ve-tos-java-sdk-shaded with all 3rd classes relocated to com.volcengine.tos. package is feasible, thought?
There was a problem hiding this comment.
Thanks @pan3793 for your reply.
AFAIK, the hadoop-client-runtime was not fully tested in the Hadoop project, and several issues caused by relocation were identified after Spark adoption. I think the hadoop-tos-shade stays in the same situation
This is a new input for me. Could you help giving more details about this issue, thanks a lot.
How hard is it for TOS to switch to another HTTP client?
I'm not sure whether we can push the tos sdk to switch to another HTTP client. Even they would like to do this, it will take a lot of time. So I'd prefer to use the existed tos sdk.
publish ve-tos-java-sdk-shaded, and downstream projects like Hadoop simply consume the single jar, this will address the test coverage concern I mentioned above and save a lot of users for handling class conflict.
In the current version, the ve-tos-java-sdk-shaded is done by hadoop-tos-shade, and the hadoop-tos-core module depends on the hadoop-tos-shade.
If I publish a ve-tos-java-sdk-shaded, what is the difference between the published ve-tos-java-sdk-shaded ant the orignal hadoo-tos-shade module? May be because I don't understand the reflection stuff, I'm not clear at here. Please help if I'm wrong, thanks a lot.
I prefer to make hadoop-tos depending on tos sdk directly, without any shade. The users can shade hadoop-tos and it's dependencies if necessary. This should be the most flexible way, without any side effect.
There was a problem hiding this comment.
Could you help giving more details about this issue
I just pick some tickets to demonstrate the issue, there may be other issues I missed or are not identified yet
- https://issues.apache.org/jira/browse/HADOOP-16080
- https://issues.apache.org/jira/browse/HADOOP-17324
- https://issues.apache.org/jira/browse/HADOOP-17891
If I publish a ve-tos-java-sdk-shaded, what is the difference between the published ve-tos-java-sdk-shaded ant the orignal hadoo-tos-shade module?
have you checked the error stacktrace of unit/integration tests, which package show? the vanilla kotlin package or the relocated one?
AFAIK, maven has limited support consuming a shaded module in the same project, that's why hadoop and some other apache projects which use maven as the building system have a dedicated repo for 3rd relocated libs https://github.com/apache/hadoop-thirdparty
wojiaodoubao
force-pushed
the
HADOOP-19236-01
branch
from
0d3b07c to
413f3e7
Compare
|
💔 -1 overall
This message was automatically generated. |
wojiaodoubao
force-pushed
the
HADOOP-19236-01
branch
from
413f3e7 to
a62d2ae
Compare
|
💔 -1 overall
This message was automatically generated. |
…ntation. Contributed by: ZhengHu, SunXin, XianyinXin, Rascal Wu, FangBo, Yuanzhihuan.
wojiaodoubao
force-pushed
the
HADOOP-19236-01
branch
from
a62d2ae to
547fb01
Compare
|
💔 -1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
Please refer to #7194.
The tos integration work is too big. Split it into 2 parts and this is the first part.
How was this patch tested?
With unit tests.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?