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

[#3924] improvemet(core): Switch default entity store from KV to relational #3954

Merged
merged 18 commits into from
Jul 1, 2024

Conversation

yuqi1129
Copy link
Contributor

What changes were proposed in this pull request?

Change configurations about default entity store and change default entity store from KV to relational.

Why are the changes needed?

We are going to make KV entity deprecated.

Fix: #3924

Does this PR introduce any user-facing change?

N/A.

How was this patch tested?

Existing test can cover it.

@yuqi1129 yuqi1129 marked this pull request as draft June 24, 2024 12:49
@yuqi1129
Copy link
Contributor Author

This PR is not ready for review until #3922 is merged.

@yuqi1129 yuqi1129 self-assigned this Jun 24, 2024
@yuqi1129 yuqi1129 marked this pull request as ready for review June 27, 2024 06:31
# gravitino.entity.store.kv = RocksDBKvBackend

gravitino.entity.store = relational
gravitino.entity.store.relational = JDBCBackend
# The storage path for RocksDB storage implementation, it supports both absolute and relative path,
# If the value is a relative path, the final path is "${GRAVITINO_HOME}/${PATH_YOU_HAVA_SET}", default value
# is "${GRAVITINO_HOME}/data/rocksdb", please uncomment and change it in your production environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove any kv, rocksdb related wordings here, as this is already deprecated, we should remove form configuration file and use h2 as a default one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it

@@ -149,14 +149,14 @@ Integer updateCatalogMeta(
@Update(
"UPDATE "
+ TABLE_NAME
+ " SET deleted_at = UNIX_TIMESTAMP(CURRENT_TIMESTAMP(3)) * 1000.0"
+ " SET deleted_at =(UNIX_TIMESTAMP() * 1000.0) + EXTRACT(MICROSECOND FROM CURRENT_TIMESTAMP(3)) / 1000"
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't remove the whitespace here. Also can you please check it this EXTRACT(MICROSECOND FROM CURRENT_TIMESTAMP(3) is supported by other databases.

Copy link
Contributor Author

@yuqi1129 yuqi1129 Jun 27, 2024

Choose a reason for hiding this comment

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

I have checked: EXTRACT(MICROSECOND FROM CURRENT_TIMESTAMP(3) does not work in PG, maybe we need to change the SQL accordingly.

The reason why I modified here is that for H2, UNIX_TIMESTAMP(CURRENT_TIMESTAMP(3)) * 1000.0 returns a full second time instead of a millisecond time, which is quite different from MySQL, so I changed it.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this sql dialect only supported by for H2, MySQL, how do you handle other embedded database or other databases like PG? You will definitely have a chance to meet microsecond issues even not for embedded database.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You will definitely have a chance to meet microsecond issues even for embedded database.

Yeah, I would be another issue. I need these changes to make SQL between H2 and MySQL work now. When It comes to other dialects like PG or Oracle, we need to design a mechanism that can be adapted to several different databases. This seems to be beyond the scope of this PR.

@yuqi1129 yuqi1129 requested a review from jerryshao June 27, 2024 13:14
@@ -149,14 +149,14 @@ Integer updateCatalogMeta(
@Update(
"UPDATE "
+ TABLE_NAME
+ " SET deleted_at = UNIX_TIMESTAMP(CURRENT_TIMESTAMP(3)) * 1000.0"
+ " SET deleted_at = (UNIX_TIMESTAMP() * 1000.0) + EXTRACT(MICROSECOND FROM CURRENT_TIMESTAMP(3)) / 1000"
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it is too long to exceed 100 chars, please check and fix them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@jerryshao jerryshao merged commit f39fa4a into apache:main Jul 1, 2024
33 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.

Switch default entity backend to embedded JDBC
2 participants