dimas-b commented on code in PR #1456:
URL: https://github.com/apache/polaris/pull/1456#discussion_r2060950517
##########
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java:
##########
@@ -1843,6 +1845,55 @@ public void
testConcurrencyConflictUpdateTableDuringFinalTransaction() {
.hasMessageContaining("conflict_table");
}
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ public void testTableOperationsDoesNotRefreshAfterCommit(boolean
updateMetadataOnCommit) {
+ if (this.requiresNamespaceCreate()) {
Review Comment:
This is always true for Polaris, is it not?
##########
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1476,6 +1493,17 @@ public void doCommit(TableMetadata base, TableMetadata
metadata) {
+ "because it has been concurrently modified to %s",
tableIdentifier, oldLocation, newLocation, existingLocation);
}
+
+ // We diverge from `BaseMetastoreTableOperations` in the below code block
+ if (updateMetadataOnCommit) {
Review Comment:
"update" sounds very intrusive to me, and it feel related to the metadata in
storage... How about `makeMetadataCurrentOnCommit`?
##########
polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java:
##########
@@ -59,4 +59,14 @@ 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 update the metadata
that is passed into"
+ + " `commit`, and re-use it to skip a trip to object storage
to re-construct"
+ + " the committed metadata again.")
Review Comment:
TBH, I find the description confusing. Why should the user (admin) have to
know which specific classes use this option? What does "update the metadata"
mean? It is hard to get what's involved without knowing the related code.
##########
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java:
##########
@@ -1693,8 +1695,8 @@ public void testFileIOWrapper() {
table.updateProperties().set("foo", "bar").commit();
Assertions.assertThat(measured.getInputBytes())
- .as("A table was read and written")
- .isGreaterThan(0);
+ .as("A table was read and written, but no trip to storage was made")
Review Comment:
nit: isn't the message supposed to describe failures as opposed to expected
situation?
--
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]
