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.

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

[Improvement] Rethinking the implement of partition operations #2999

Closed
FANNG1 opened this issue Apr 17, 2024 · 5 comments
Closed

[Improvement] Rethinking the implement of partition operations #2999

FANNG1 opened this issue Apr 17, 2024 · 5 comments
Assignees
Labels
improvement Improvements on everything

Comments

@FANNG1
Copy link
Contributor

FANNG1 commented Apr 17, 2024

What would you like to be improved?

The current partition operation is out of dispatcher, leading to some issues:

  1. [FEATURE] Provide specific classLoader for Partition operation #2997
  2. blocks [#2780] Improvement(server,core): Move tree lock from rest api to the corresponding implementation to minimize tree lock range. #2873
  3. makes event listener hard to inject to partition operations.
                  Table loadTable = dispatcher.loadTable(tableIdent);
                  Partition p = loadTable.supportPartitions().getPartition(partition);
                  return Utils.ok(new PartitionResponse(DTOConverters.toDTO(p)));
```to 
@FANNG1 FANNG1 added the improvement Improvements on everything label Apr 17, 2024
@FANNG1
Copy link
Contributor Author

FANNG1 commented Apr 17, 2024

cc @jerryshao @mchades @yuqi1129

@jerryshao
Copy link
Contributor

@mchades please take a look.

@jerryshao
Copy link
Contributor

I think it is necessary to refactor this part, we have to carefully rethink the whole thing.

BTW, I'm curious why Hive's partition operation doesn't have such problem like #2997 mentioned.

@FANNG1
Copy link
Contributor Author

FANNG1 commented Apr 17, 2024

I think it is necessary to refactor this part, we have to carefully rethink the whole thing.

BTW, I'm curious why Hive's partition operation doesn't have such problem like #2997 mentioned.

I guess Hive partition operations doesn't invoke the DynConstructor to load a new class. cc @TEOTEO520

@yuqi1129
Copy link
Contributor

I think it is necessary to refactor this part, we have to carefully rethink the whole thing.

BTW, I'm curious why Hive's partition operation doesn't have such problem like #2997 mentioned.

It's because the classDynConstructors that Hive catalog use is our class NOT class provided by Iceberg

please see: "com.datastrato.gravitino.catalog.hive.dyn.DynConstructors"

@jerryshao jerryshao added this to the Gravitino June Release milestone Apr 24, 2024
jerryshao pushed a commit that referenced this issue May 9, 2024
…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
…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
yuqi1129 pushed a commit that referenced this issue May 9, 2024
…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 <liminghuang@datastrato.com>
diqiu50 pushed a commit to diqiu50/gravitino that referenced this issue Jun 13, 2024
…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
Labels
improvement Improvements on everything
Projects
None yet
Development

No branches or pull requests

4 participants