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

[#2856] fix(core): Fix the namespace missing issue when get entities from kv storage #2971

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

jerryshao
Copy link
Contributor

What changes were proposed in this pull request?

This PR fixes the namespace missing issue when entities are gotten from kv storage.

Why are the changes needed?

Fix: #2856

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Modified UTs.

@jerryshao jerryshao requested review from mchades and qqqttt123 April 16, 2024 09:26
@jerryshao jerryshao self-assigned this Apr 16, 2024
@jerryshao jerryshao requested a review from yuqi1129 April 16, 2024 09:26
@yuqi1129
Copy link
Contributor

@jerryshao

I noticed that you only set the namespace value when deserialization and will not store the value in the key-value store. My question is: Why do we have to change the interface EntitySerDe instead of setting it(namespace value) in the caller method?

@@ -539,7 +539,8 @@ tasks.rat {
"**/LICENSE.*",
"**/NOTICE.*",
"ROADMAP.md",
"clients/client-python/.pytest_cache/*"
"clients/client-python/.pytest_cache/*",
"clients/client-python/gravitino.egg-info/*"
Copy link
Contributor

Choose a reason for hiding this comment

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

This folder does not exist on my local machine, can anyone help to confirm this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Try gradle build.

@jerryshao
Copy link
Contributor Author

@jerryshao

I noticed that you only set the namespace value when deserialization and will not store the value in the key-value store. My question is: Why do we have to change the interface EntitySerDe instead of setting it(namespace value) in the caller method?

You can check the issue to know the details. Simply, it is because the deserialized entity from KV has an empty namespace, but for catalog like kafka needs to get namespace from entity to use, and it cannot get a namespace from context (caller), you can see KafkaOperation's CatalogInfo (CatalogEntity).

Besides, for relational store, they all have namespace set when get from interface, so it is also a behavior alignment.

@yuqi1129
Copy link
Contributor

@jerryshao

I noticed that you only set the namespace value when deserialization and will not store the value in the key-value store. My question is: Why do we have to change the interface EntitySerDe instead of setting it(namespace value) in the caller method?

You can check the issue to know the details. Simply, it is because the deserialized entity from KV has an empty namespace, but for catalog like kafka needs to get namespace from entity to use, and it cannot get a namespace from context (caller), you can see KafkaOperation's CatalogInfo (CatalogEntity).

Besides, for relational store, they all have namespace set when get from interface, so it is also a behavior alignment.

I see, let me check again

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.

It's okay with me.

Copy link
Contributor

@mchades mchades left a comment

Choose a reason for hiding this comment

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

LGTM

@mchades mchades merged commit e2d1fd0 into apache:main Apr 17, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] Get entity from KV backend missing namespace
3 participants