-
Notifications
You must be signed in to change notification settings - Fork 380
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
[#2083] feat(core): Add JDBC backend operations for table #2384
Conversation
20c00ad
to
5061ed8
Compare
@yuqi1129 @jerryshao @qqqttt123 This PR is ready for reviews, please take a look when you have time, thanks! |
KEY `idx_mid` (`metalake_id`), | ||
KEY `idx_cid` (`catalog_id`) | ||
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin COMMENT 'table metadata'; |
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 remembered that for table
metadata, you will have another "version" table, am I right or will you do it in another PR?
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.
There is no need to create a version
table for the properties that TableEntity
currently contains, so I did not create a version
table in this PR. If TableEntity
needs to add more properties and maintain multiple versions in the future, we can create the version
table separately. For FilesetEntity
I will create a version
table in the init PR, because it has properties such as Comment
, Properties
and StorageLocation
that require multi-version management.
This is the current properties of TableEntity
:
And this is the current properties of FilesetEntity
:
@yuqi1129 @qqqttt123 would you please help to review this when you have time? |
} else { | ||
throw new IOException("Failed to update the entity: " + identifier); | ||
} | ||
} |
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.
IIUC this update operation seems not transactional, but it's fine as we have locks in the outside.
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 learned that the current plan is to implement locks in the upper layer, because backends without transaction capabilities may be supported, so more complex locks are not implemented here.
core/src/main/java/com/datastrato/gravitino/storage/relational/utils/POConverters.java
Outdated
Show resolved
Hide resolved
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.
Overall LGTM, @yuqi1129 @qqqttt123 please take time to review this.
### What changes were proposed in this pull request? The purpose of this PR is to implement JDBC backend operation for Fileset metadata. Depend on #2384. ### Why are the changes needed? Fix: #2084 ### How was this patch tested? Add unit tests to test the fileset metadata ops. --------- Co-authored-by: xiaojiebao <[email protected]>
What changes were proposed in this pull request?
The purpose of this PR is to implement JDBC backend operation for Table metadata. Depend on #2377 . Metadata operations of Fileset will be supported in the remaining PRs.
Why are the changes needed?
Fix: #2083
How was this patch tested?
Add unit tests to test the table metadata ops.