-
Notifications
You must be signed in to change notification settings - Fork 383
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
Conversation
This PR is not ready for review until #3922 is merged. |
conf/gravitino.conf.template
Outdated
# 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@@ -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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
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.