eric-maynard commented on code in PR #1378:
URL: https://github.com/apache/polaris/pull/1378#discussion_r2047936123
##########
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1328,6 +1344,25 @@ public void doCommit(TableMetadata base, TableMetadata
metadata) {
String newLocation = writeNewMetadataIfRequired(base == null, metadata);
String oldLocation = base == null ? null : base.metadataFileLocation();
+ // TODO: we should not need to do this hack, but there's no other way to
modify
+ // metadataFileLocation / currentMetadataLocation
+ if (updateMetadataOnCommit) {
+ try {
+ Field tableMetadataField =
TableMetadata.class.getDeclaredField("metadataFileLocation");
+ tableMetadataField.setAccessible(true);
+ tableMetadataField.set(metadata, newLocation);
Review Comment:
No worries, this was a pretty nuanced bug to pin down.
The basic flow is like this:
1. `IcebergCatalog.registerTable`
2. `BaseMetastoreTableOperations.commit`
a. `BasePolarisTableOperations.doCommit`
b. `BaseMetastoreTableOperations.requestRefresh`
We do not override `commit` in `BasePolarisTableOperations`, though I tried
that in an earlier iteration of this fix. Ultimately after chatting with
@singhpk234 I came to realize that `BaseMetastoreTableOperations` already has
logic that's meant to handle this exact case -- this line
[here](https://github.com/apache/iceberg/blob/013d09e473faada1a3b21c155964ec4d1254d69d/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java#L189).
However, the bug is that `currentMetadataLocation` does not properly get set
during commit, so this check never actually lets `doRefresh` skip the trip to
object storage.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
