You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "ebyhr (via GitHub)" <gi...@apache.org> on 2025/10/18 11:11:05 UTC
[PR] Core: Check table UUID in RESTTableOperations [iceberg]
ebyhr opened a new pull request, #14363:
URL: https://github.com/apache/iceberg/pull/14363
Fixes #14337
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Check table UUID in RESTTableOperations [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14363:
URL: https://github.com/apache/iceberg/pull/14363#discussion_r2442509676
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -175,12 +175,25 @@ private TableMetadata updateCurrentMetadata(LoadTableResponse response) {
// safely ignored. there is no requirement to update config on refresh or commit.
if (current == null
|| !Objects.equals(current.metadataFileLocation(), response.metadataLocation())) {
- this.current = response.tableMetadata();
+ this.current = checkUUID(current, response.tableMetadata());
}
return current;
}
+ private static TableMetadata checkUUID(TableMetadata currentMetadata, TableMetadata newMetadata) {
+ String newUUID = newMetadata.uuid();
+ if (currentMetadata != null && currentMetadata.uuid() != null && newUUID != null) {
+ Preconditions.checkState(
+ newUUID.equals(currentMetadata.uuid()),
+ "Table UUID does not match: current=%s != refreshed=%s",
+ currentMetadata.uuid(),
+ newUUID);
+ }
+
+ return newMetadata;
Review Comment:
[optional to consider] wonder if we can move this to some place common `validate(current, newMetadata)` so that both BaseMetaStoreOperation and TableOperation (may be an util or just default impl of TableOps) can share it, but seems too much for ROI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Check table UUID in RESTTableOperations [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14363:
URL: https://github.com/apache/iceberg/pull/14363#discussion_r2442563678
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -175,12 +175,25 @@ private TableMetadata updateCurrentMetadata(LoadTableResponse response) {
// safely ignored. there is no requirement to update config on refresh or commit.
if (current == null
|| !Objects.equals(current.metadataFileLocation(), response.metadataLocation())) {
- this.current = response.tableMetadata();
+ this.current = checkUUID(current, response.tableMetadata());
Review Comment:
+1 for this fix. Also +1 for adding a test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Check table UUID in RESTTableOperations [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #14363:
URL: https://github.com/apache/iceberg/pull/14363#issuecomment-3483350867
Thanks @ebyhr for the PR! Thanks @singhpk234 for the review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Check table UUID in RESTTableOperations [iceberg]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on PR #14363:
URL: https://github.com/apache/iceberg/pull/14363#issuecomment-3483238574
@singhpk234 @huaxingao Can we merge this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Check table UUID in RESTTableOperations [iceberg]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on code in PR #14363:
URL: https://github.com/apache/iceberg/pull/14363#discussion_r2442705456
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -175,12 +175,25 @@ private TableMetadata updateCurrentMetadata(LoadTableResponse response) {
// safely ignored. there is no requirement to update config on refresh or commit.
if (current == null
|| !Objects.equals(current.metadataFileLocation(), response.metadataLocation())) {
- this.current = response.tableMetadata();
+ this.current = checkUUID(current, response.tableMetadata());
Review Comment:
Thanks for the suggestion. Added a unit test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Check table UUID in RESTTableOperations [iceberg]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on PR #14363:
URL: https://github.com/apache/iceberg/pull/14363#issuecomment-3461054701
Rebased on main to resolve conflicts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Check table UUID in RESTTableOperations [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14363:
URL: https://github.com/apache/iceberg/pull/14363#discussion_r2442509676
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -175,12 +175,25 @@ private TableMetadata updateCurrentMetadata(LoadTableResponse response) {
// safely ignored. there is no requirement to update config on refresh or commit.
if (current == null
|| !Objects.equals(current.metadataFileLocation(), response.metadataLocation())) {
- this.current = response.tableMetadata();
+ this.current = checkUUID(current, response.tableMetadata());
}
return current;
}
+ private static TableMetadata checkUUID(TableMetadata currentMetadata, TableMetadata newMetadata) {
+ String newUUID = newMetadata.uuid();
+ if (currentMetadata != null && currentMetadata.uuid() != null && newUUID != null) {
+ Preconditions.checkState(
+ newUUID.equals(currentMetadata.uuid()),
+ "Table UUID does not match: current=%s != refreshed=%s",
+ currentMetadata.uuid(),
+ newUUID);
+ }
+
+ return newMetadata;
Review Comment:
[optional to consider] wonder if we can move this to some place common `validate(current, newMetadata)` so that both BaseMetaStoreOperation and TableOperation (may be just put it in TableOperation default impl) can share it, but seems too much for ROI.
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -175,12 +175,25 @@ private TableMetadata updateCurrentMetadata(LoadTableResponse response) {
// safely ignored. there is no requirement to update config on refresh or commit.
if (current == null
|| !Objects.equals(current.metadataFileLocation(), response.metadataLocation())) {
- this.current = response.tableMetadata();
+ this.current = checkUUID(current, response.tableMetadata());
Review Comment:
This seems reasonable to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Check table UUID in RESTTableOperations [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao merged PR #14363:
URL: https://github.com/apache/iceberg/pull/14363
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Check table UUID in RESTTableOperations [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14363:
URL: https://github.com/apache/iceberg/pull/14363#discussion_r2446224233
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -2884,6 +2885,48 @@ public void testETagWithRegisterTable() {
assertThat(respHeaders).containsEntry(HttpHeaders.ETAG, eTag);
}
+ @Test
+ void testDifferentTableUUID() {
+ RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
+ RESTCatalog catalog = catalog(adapter);
+
+ catalog.createNamespace(TABLE.namespace());
+ catalog.createTable(TABLE, SCHEMA);
+
+ // inject the wrong table UUID
Review Comment:
nit: simulate drop and re-create the table with same name
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org