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

[#5633] remove the key-value storage backend logic #5645

Merged
merged 4 commits into from
Dec 10, 2024
Merged

[#5633] remove the key-value storage backend logic #5645

merged 4 commits into from
Dec 10, 2024

Conversation

pithecuse527
Copy link
Contributor

What changes were proposed in this pull request?

Remove Key-Value storage backend storage implementations

Why are the changes needed?

Fix: #5633

Does this PR introduce any user-facing change?

yes

How was this patch tested?

Remove tests for the Key-Value storage implementation code

@jerryshao
Copy link
Contributor

Thanks @pithecuse527 for the contribution. I think the meta module is also not needed any more, and also the related codes like serde, you can remove them all.

@jerryshao jerryshao requested a review from yuqi1129 November 22, 2024 10:17
@pithecuse527
Copy link
Contributor Author

@jerryshao

Hello, I have removed the module meta and its related implementations.

@jerryshao
Copy link
Contributor

@yuqi1129 please spend time on this PR, to help @pithecuse527 to get this done, thanks.

@yuqi1129
Copy link
Contributor

yuqi1129 commented Nov 26, 2024 •
edited
Loading

@pithecuse527 , Hi, thanks for your contribution, I have checked the code and the following can be further improved

  • The following class can also be dropped.
image
  • The docs can be detected
image
  • We can still remove some classes like EntitySerDe

Also, please carefully check the code, I notice that RocksDB JNI is still in some Licese files.

@pithecuse527
Copy link
Contributor Author

pithecuse527 commented Nov 26, 2024 •
edited
Loading

@yuqi1129

Thanks for the help.

As I go through, I wonder why we need to delete the RandomIdGenerator class.
I think it is needed for the GravitinoEnv?

@yuqi1129
Copy link
Contributor

@yuqi1129

Thanks for the help.

As I go through, I wonder why we need to delete the RandomIdGenerator class. I think it is needed for the GravitinoEnv?

Oh, my fault, IdGenerator and RandomIdGenerator are both needed by Gravitino, we should keep them.

@pithecuse527
Copy link
Contributor Author

pithecuse527 commented Nov 27, 2024 •
edited
Loading

@yuqi1129
I have applied your feedback. But one more question.

Also, please carefully check the code, I notice that RocksDB JNI is still in some Licese files.

I don't see any RocksDB JNI content in the other license files. Could you guide me on what I should be looking for?

If everything else looks fine, I can rebase this branch.

@yuqi1129
Copy link
Contributor

@yuqi1129 I have applied your feedback. But one more question.

Also, please carefully check the code, I notice that RocksDB JNI is still in some Licese files.

I don't see any RocksDB JNI content in the other license files. Could you guide me on what I should be looking for?

If everything else looks fine, I can rebase this branch.

I see, there are some build package that contains content RocksDB JNI, so please ignore this one.

@pithecuse527 pithecuse527 force-pushed the pithecuse527/remove-kv-storage-backend-5633 branch from 0751646 to d533bb3 Compare November 27, 2024 02:35
@pithecuse527
Copy link
Contributor Author

Thanks for letting me know.

I’ve rebased this branch.
Feel free to share any further comments or issues.

yuqi1129
yuqi1129 reviewed Nov 29, 2024
View reviewed changes
Copy link
Contributor

@yuqi1129 yuqi1129 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. @jerryshao would you like to take a look?

@jerryshao
Copy link
Contributor

Looks like we still have some build issues, @pithecuse527 would you please make the CI pass?

@pithecuse527 pithecuse527 force-pushed the pithecuse527/remove-kv-storage-backend-5633 branch 2 times, most recently from 46cae48 to a42fae7 Compare December 4, 2024 09:14
@yuqi1129 yuqi1129 closed this Dec 4, 2024
@yuqi1129 yuqi1129 reopened this Dec 4, 2024
@pithecuse527 pithecuse527 force-pushed the pithecuse527/remove-kv-storage-backend-5633 branch from a42fae7 to deae27e Compare December 4, 2024 10:09
@jerryshao
Copy link
Contributor

Looks like the CI is passed, @yuqi1129 can you please help to check if everything is OK?

@yuqi1129
Copy link
Contributor

yuqi1129 commented Dec 4, 2024

Looks like the CI is passed, @yuqi1129 can you please help to check if everything is OK?

Got it.

@pithecuse527
Copy link
Contributor Author

pithecuse527 commented Dec 4, 2024 •
edited
Loading

Feel free to let me know if there are further requests :)

Thank you for the guidance. @yuqi1129 @jerryshao

yuqi1129
yuqi1129 reviewed Dec 4, 2024
View reviewed changes
Copy link
Contributor

@yuqi1129 yuqi1129 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor one, others look good to me.

@pithecuse527 pithecuse527 force-pushed the pithecuse527/remove-kv-storage-backend-5633 branch from deae27e to 8658c82 Compare December 8, 2024 03:59
jerryshao
jerryshao approved these changes Dec 10, 2024
View reviewed changes
Copy link
Contributor

@jerryshao jerryshao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @pithecuse527 for your contribution.

@yuqi1129 can you please take another look?

@yuqi1129
Copy link
Contributor

Merge it into main, @pithecuse527 Thanks for your hard work.

@yuqi1129 yuqi1129 merged commit aebb9d2 into apache:main Dec 10, 2024
26 checks passed
@pithecuse527 pithecuse527 deleted the pithecuse527/remove-kv-storage-backend-5633 branch December 10, 2024 14:45
Abyss-lord pushed a commit to Abyss-lord/gravitino that referenced this pull request Dec 13, 2024
@pithecuse527 @Abyss-lord
[apache#5633] remove the key-value storage backend logic (apache#5645)
61769a6 
<!--
1. Title: [#<issue>] <type>(<scope>): <subject>
   Examples:
     - "[apache#123] feat(operator): support xxx"
     - "[apache#233] fix: check null before access result in xxx"
     - "[MINOR] refactor: fix typo in variable name"
     - "[MINOR] docs: fix typo in README"
     - "[apache#255] test: fix flaky test NameOfTheTest"
   Reference: https://www.conventionalcommits.org/en/v1.0.0/
2. If the PR is unfinished, please mark this PR as draft.
-->

### What changes were proposed in this pull request?
Remove Key-Value storage backend storage implementations

### Why are the changes needed?

Fix: apache#5633

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

### How was this patch tested?
Remove tests for the Key-Value storage implementation code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tengqm tengqm tengqm left review comments

@yuqi1129 yuqi1129 yuqi1129 approved these changes

@jerryshao jerryshao jerryshao approved these changes

Assignees

@pithecuse527 pithecuse527

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Improvement] Remove RocksDBKvBackend and related code & dependencies from code base.
4 participants
@pithecuse527 @jerryshao @yuqi1129 @tengqm