adnanhemani commented on code in PR #433:
URL: https://github.com/apache/polaris/pull/433#discussion_r2098853001
##########
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1297,22 +1306,35 @@ public TableMetadata refresh() {
return current();
}
+ /**
+ * With metadata caching, the `base` may not be exactly `current()` by
reference so we compare
+ * locations instead
+ */
@Override
public void commit(TableMetadata base, TableMetadata metadata) {
// if the metadata is already out of date, reject it
- if (base != current()) {
- if (base != null) {
- throw new CommitFailedException("Cannot commit: stale table
metadata");
- } else {
+ if (base == null) {
+ if (current() != null) {
// when current is non-null, the table exists. but when base is
null, the commit is trying
// to create the table
throw new AlreadyExistsException("Table already exists: %s",
fullTableName);
}
+ } else if (current() != null
+ &&
!current().metadataFileLocation().equals(base.metadataFileLocation())) {
+ throw new CommitFailedException("Cannot commit: stale table metadata");
}
// if the metadata is not changed, return early
if (base == metadata) {
LOGGER.info("Nothing to commit.");
return;
+ } else {
Review Comment:
nit: use "else if"?
##########
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -1105,21 +1117,20 @@ public void renameView(RenameTableRequest request) {
catalogHandlerUtils.renameView(viewCatalog, request);
}
+ private @Nonnull TableMetadata filterMetadataToSnapshots(
+ TableMetadata metadata, String snapshots) {
Review Comment:
Where is the `snapshots` variable being used?
##########
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1501,24 +1506,50 @@ public void doCommit(TableMetadata base, TableMetadata
metadata) {
IcebergTableLikeEntity entity =
IcebergTableLikeEntity.of(resolvedPath == null ? null :
resolvedPath.getRawLeafEntity());
String existingLocation;
+ int maxMetadataCacheBytes =
+ callContext
+ .getPolarisCallContext()
+ .getConfigurationStore()
+ .getConfiguration(
+ callContext.getPolarisCallContext(),
+ catalogEntity,
+ FeatureConfiguration.METADATA_CACHE_MAX_BYTES);
+ Optional<String> metadataJsonToCache =
+ switch (maxMetadataCacheBytes) {
+ case
FeatureConfiguration.Constants.METADATA_CACHE_MAX_BYTES_NO_CACHING ->
+ Optional.empty();
+ case
FeatureConfiguration.Constants.METADATA_CACHE_MAX_BYTES_INFINITE_CACHING -> {
Review Comment:
why is this stylistically different than the case above it?
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
