Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[FEATURE] Provide specific classLoader for Partition operation #2997

Closed
TEOTEO520 opened this issue Apr 17, 2024 · 7 comments · Fixed by #3221
Closed

[FEATURE] Provide specific classLoader for Partition operation #2997

TEOTEO520 opened this issue Apr 17, 2024 · 7 comments · Fixed by #3221
Assignees
@mchades @TEOTEO520
Labels
feature New feature or request
Milestone
Gravitino June Re...

Comments

@TEOTEO520
Copy link
Contributor

Describe the feature

There is no specific catalog classLoader for partition operation, if there is a partition operation need to use Thread.currentThread().getContextClassLoader() to get classLoader, it will get WebAppClassLoader, and this will cause ClassNotFoundException
image

Motivation

No response

Describe the solution

Provide specific classLoader for Partition operation

Additional context

No response
@TEOTEO520 TEOTEO520 added the feature New feature or request label Apr 17, 2024
@yuqi1129
Copy link
Contributor

Can you provide more details and additional stacks?

@TEOTEO520
Copy link
Contributor Author

Like iceberg partition operation, it will get current thread classLoader to operate
image
image

@yuqi1129
Copy link
Contributor

yuqi1129 commented Apr 17, 2024 •
edited
Loading

Iceberg catalog-related classes are loaded by IsolatedClassloader, so it seems that the class loader can't be WebAppClassLoader.

@TEOTEO520
Copy link
Contributor Author

image

@TEOTEO520
Copy link
Contributor Author

@mchades also agree that the partition operation need to be separate, you can ask him for details, if community decide to do it, please assign it for me

@FANNG1
Copy link
Contributor

FANNG1 commented Apr 17, 2024 •
edited
Loading

@TEOTEO520 , thanks for reporting this issue, does hive work well for partition operations?

@TEOTEO520
Copy link
Contributor Author

@TEOTEO520 , thanks for reporting this issue, does hive work well for partition operations?

Yes, but when hive do the operation of partition, it also use WebAppClassLoader, I'm confused it does work well
image

@FANNG1 FANNG1 mentioned this issue Apr 17, 2024
Closed
@jerryshao jerryshao added this to the Gravitino June Release milestone Apr 24, 2024
@mchades mchades mentioned this issue Apr 29, 2024
Merged
jerryshao pushed a commit that referenced this issue May 9, 2024
@mchades
[#2999][#2997] improvement(partition): add partition operation dispat…
09f82cf 
…cher (#3221)

### What changes were proposed in this pull request?

- reuse the class loader of the catalog for partition operation
- add partition operation dispatcher

### Why are the changes needed?

Fix: #2997 #2999 

### Does this PR introduce _any_ user-facing change?

no

### How was this patch tested?

tests added
github-actions bot pushed a commit that referenced this issue May 9, 2024
@mchades @web-flow
[#2999][#2997] improvement(partition): add partition operation dispat…
ff1d39e 
…cher (#3221)

### What changes were proposed in this pull request?

- reuse the class loader of the catalog for partition operation
- add partition operation dispatcher

### Why are the changes needed?

Fix: #2997 #2999 

### Does this PR introduce _any_ user-facing change?

no

### How was this patch tested?

tests added
@jerryshao jerryshao mentioned this issue May 9, 2024
Merged
yuqi1129 pushed a commit that referenced this issue May 9, 2024
@jerryshao @mchades
[#2999][#2997] improvement(partition): add partition operation dispat…
c543810 
…cher (#3315)

### What changes were proposed in this pull request?

- reuse the class loader of the catalog for partition operation
- add partition operation dispatcher

### Why are the changes needed?

Fix: #2997 #2999 

### Does this PR introduce _any_ user-facing change?

no

### How was this patch tested?

tests added

Co-authored-by: mchades <[email protected]>
diqiu50 pushed a commit to diqiu50/gravitino that referenced this issue Jun 13, 2024
@mchades @diqiu50
[apache#2999][apache#2997] improvement(partition): add partition oper…
e78797c 
…ation dispatcher (apache#3221)

### What changes were proposed in this pull request?

- reuse the class loader of the catalog for partition operation
- add partition operation dispatcher

### Why are the changes needed?

Fix: apache#2997 apache#2999 

### Does this PR introduce _any_ user-facing change?

no

### How was this patch tested?

tests added
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mchades mchades

@TEOTEO520 TEOTEO520

Labels
feature New feature or request
Projects
None yet
Milestone
Gravitino June Release
Development

Successfully merging a pull request may close this issue.

[#2999][#2997] improvement(partition): add partition operation dispatcher mchades/gravitino
5 participants
@jerryshao @FANNG1 @yuqi1129 @mchades @TEOTEO520