dimas-b commented on code in PR #1378:
URL: https://github.com/apache/polaris/pull/1378#discussion_r2047821651
##########
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);
Review Comment:
This should probably be done in a `static` init method.
##########
polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java:
##########
@@ -59,4 +59,13 @@ protected BehaviorChangeConfiguration(
.description("Whether or not to use soft values in the entity cache")
.defaultValue(false)
.buildBehaviorChangeConfiguration();
+
+ public static final BehaviorChangeConfiguration<Boolean>
TABLE_OPERATIONS_COMMIT_UPDATE_METADATA =
+ PolarisConfiguration.<Boolean>builder()
+ .key("TABLE_OPERATIONS_COMMIT_UPDATE_METADATA")
+ .description(
+ "If true, BasePolarisTableOperations should cache metadata that
has been committed"
Review Comment:
based on this description I'd propose to rename the property to
`CACHE_COMMITTED_METADATA`.
##########
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1184,16 +1197,19 @@ public ViewBuilder withLocation(String newLocation) {
}
}
- private class BasePolarisTableOperations extends
BaseMetastoreTableOperations {
+ public class BasePolarisTableOperations extends BaseMetastoreTableOperations
{
private final TableIdentifier tableIdentifier;
private final String fullTableName;
+ private final boolean updateMetadataOnCommit;
private FileIO tableFileIO;
- BasePolarisTableOperations(FileIO defaultFileIO, TableIdentifier
tableIdentifier) {
+ BasePolarisTableOperations(
+ FileIO defaultFileIO, TableIdentifier tableIdentifier, boolean
updateMetadataOnCommit) {
LOGGER.debug("new BasePolarisTableOperations for {}", tableIdentifier);
this.tableIdentifier = tableIdentifier;
this.fullTableName = fullTableName(catalogName, tableIdentifier);
this.tableFileIO = defaultFileIO;
+ this.updateMetadataOnCommit = updateMetadataOnCommit;
Review Comment:
Why not do the same for views?
##########
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:
I believe we should avoid modifying arguments passed to this method,
especially in ways that can hardly be expected by the caller.
Is the storage call we're trying to avoid relevant in production runtime of
Polaris or only in tests?
--
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]
