You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "ajantha-bhat (via GitHub)" <gi...@apache.org> on 2023/05/18 09:33:58 UTC
[GitHub] [iceberg] ajantha-bhat opened a new pull request, #7641: Nessie: Disable table metadata files cleanup during commits
ajantha-bhat opened a new pull request, #7641:
URL: https://github.com/apache/iceberg/pull/7641
Avoid automatic cleanup of table metadata files during commit as it can be used by other references.
--
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
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #7641: Nessie: Disable table metadata files cleanup during commits
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #7641:
URL: https://github.com/apache/iceberg/pull/7641#discussion_r1198972111
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java:
##########
@@ -92,6 +92,18 @@ private TableMetadata loadTableMetadata(String metadataLocation, Reference refer
// Nessie CLI will provide a reference aware GC functionality for the expired/unreferenced
// files.
newProperties.put(TableProperties.GC_ENABLED, "false");
+
+ boolean metadataCleanupEnabled =
+ newProperties
+ .getOrDefault(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "false")
+ .equalsIgnoreCase("true");
+ if (metadataCleanupEnabled) {
+ newProperties.put(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "false");
+ LOG.warn(
+ "Clean up of table metadata files during commit is disabled as it can be in use by other references."
+ + " Use nessie-gc tool for reference aware GC");
Review Comment:
> I guess the WARN is going to be logged on every load, unless the user permanently resets the METADATA_DELETE_AFTER_COMMIT_ENABLED property to false, right?
No, this will be set in table metadata refresh flow. So, this property will be set to false and written to file in the first refresh. So, it won't print on every load. For example, this testcase execution prints it only once.
--
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
[GitHub] [iceberg] dimas-b commented on a diff in pull request #7641: Nessie: Disable table metadata files cleanup during commits
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #7641:
URL: https://github.com/apache/iceberg/pull/7641#discussion_r1198962340
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java:
##########
@@ -92,6 +92,18 @@ private TableMetadata loadTableMetadata(String metadataLocation, Reference refer
// Nessie CLI will provide a reference aware GC functionality for the expired/unreferenced
// files.
newProperties.put(TableProperties.GC_ENABLED, "false");
+
+ boolean metadataCleanupEnabled =
+ newProperties
+ .getOrDefault(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "false")
+ .equalsIgnoreCase("true");
+ if (metadataCleanupEnabled) {
+ newProperties.put(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "false");
+ LOG.warn(
+ "Clean up of table metadata files during commit is disabled as it can be in use by other references."
+ + " Use nessie-gc tool for reference aware GC");
Review Comment:
How about `Automatic table metadata files cleanup was requested, but disabled because the Nessie catalog can use historical metadata files. Please set the " + TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED + " property to 'false'. Use the 'nessie-gc' tool for history-aware GC".`?
--
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
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #7641: Nessie: Disable table metadata files cleanup during commits
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #7641:
URL: https://github.com/apache/iceberg/pull/7641#discussion_r1198070578
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -579,6 +584,44 @@ public void testGCEnabled() {
"Cannot expire snapshots: GC is disabled (deleting files may corrupt other tables)");
}
+ @Test
+ public void testTableMetadataFilesCleanupDisable() {
+ Table icebergTable = catalog.loadTable(TABLE_IDENTIFIER);
+
+ Assertions.assertThat(
+ icebergTable.properties().get(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED))
+ .isNotNull()
+ .isEqualTo("false");
+
+ icebergTable
+ .updateProperties()
+ .set(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "true")
Review Comment:
yes. added the validation.
--
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
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #7641: Nessie: Disable table metadata files cleanup during commits
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #7641:
URL: https://github.com/apache/iceberg/pull/7641#discussion_r1198071972
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -579,6 +584,44 @@ public void testGCEnabled() {
"Cannot expire snapshots: GC is disabled (deleting files may corrupt other tables)");
}
+ @Test
+ public void testTableMetadataFilesCleanupDisable() {
+ Table icebergTable = catalog.loadTable(TABLE_IDENTIFIER);
+
+ Assertions.assertThat(
+ icebergTable.properties().get(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED))
+ .isNotNull()
+ .isEqualTo("false");
+
+ icebergTable
+ .updateProperties()
+ .set(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "true")
+ .commit();
+ icebergTable
+ .updateProperties()
+ .set(TableProperties.METADATA_PREVIOUS_VERSIONS_MAX, "1")
+ .commit();
+
+ String metadataFileLocation =
+ ((BaseTable) icebergTable).operations().current().metadataFileLocation();
+ Path metadataFileLocationPath = Paths.get(metadataFileLocation.replaceFirst("file:", ""));
Review Comment:
Can you paste the expected Assertions here? I think I didn't fully get it.
Also, other places uses this metadata location string directly
https://github.com/apache/iceberg/blob/39a5f503c1d19d41a15b74ea53e3d5e51d495874/hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java#L238
--
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
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #7641: Nessie: Disable table metadata files cleanup during commits
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #7641:
URL: https://github.com/apache/iceberg/pull/7641#discussion_r1198682022
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java:
##########
@@ -92,6 +92,11 @@ private TableMetadata loadTableMetadata(String metadataLocation, Reference refer
// Nessie CLI will provide a reference aware GC functionality for the expired/unreferenced
// files.
newProperties.put(TableProperties.GC_ENABLED, "false");
+
+ newProperties.put(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "false");
+ LOG.info(
Review Comment:
Added conditional warning logs and improved logs to contain actionable item.
--
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
[GitHub] [iceberg] dimas-b commented on a diff in pull request #7641: Nessie: Disable table metadata files cleanup during commits
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #7641:
URL: https://github.com/apache/iceberg/pull/7641#discussion_r1198977262
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java:
##########
@@ -92,6 +92,18 @@ private TableMetadata loadTableMetadata(String metadataLocation, Reference refer
// Nessie CLI will provide a reference aware GC functionality for the expired/unreferenced
// files.
newProperties.put(TableProperties.GC_ENABLED, "false");
+
+ boolean metadataCleanupEnabled =
+ newProperties
+ .getOrDefault(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "false")
+ .equalsIgnoreCase("true");
+ if (metadataCleanupEnabled) {
+ newProperties.put(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "false");
+ LOG.warn(
+ "Clean up of table metadata files during commit is disabled as it can be in use by other references."
+ + " Use nessie-gc tool for reference aware GC");
Review Comment:
SGTM :+1:
--
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
[GitHub] [iceberg] dimas-b commented on a diff in pull request #7641: Nessie: Disable table metadata files cleanup during commits
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #7641:
URL: https://github.com/apache/iceberg/pull/7641#discussion_r1198080435
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -579,6 +584,50 @@ public void testGCEnabled() {
"Cannot expire snapshots: GC is disabled (deleting files may corrupt other tables)");
}
+ @Test
+ public void testTableMetadataFilesCleanupDisable() {
+ Table icebergTable = catalog.loadTable(TABLE_IDENTIFIER);
+
+ Assertions.assertThat(
+ icebergTable.properties().get(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED))
+ .isNotNull()
+ .isEqualTo("false");
+
+ // Forceful setting of property also should get override with false
+ icebergTable
+ .updateProperties()
+ .set(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "true")
+ .commit();
+ Assertions.assertThat(
+ icebergTable.properties().get(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED))
+ .isNotNull()
+ .isEqualTo("false");
+
+ icebergTable
+ .updateProperties()
+ .set(TableProperties.METADATA_PREVIOUS_VERSIONS_MAX, "1")
+ .commit();
+
+ String metadataFileLocation =
+ ((BaseTable) icebergTable).operations().current().metadataFileLocation();
+ Path metadataFileLocationPath = Paths.get(metadataFileLocation.replaceFirst("file:", ""));
+
+ Assertions.assertThat(metadataFileLocationPath).exists();
+
+ icebergTable.updateSchema().addColumn("x1", Types.LongType.get()).commit();
+ icebergTable.updateSchema().addColumn("x2", Types.LongType.get()).commit();
+
+ // old table metadata file should still exist after commits.
+ Assertions.assertThat(metadataFileLocationPath).exists();
Review Comment:
Would it be possible to do time travel in a Nessie-specific way to get the expected metadata location? Current test looks a bit magical. It asserts that some old metadata file exists, but it does not link it to Nessie behaviour.
--
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
[GitHub] [iceberg] dimas-b commented on a diff in pull request #7641: Nessie: Disable table metadata files cleanup during commits
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #7641:
URL: https://github.com/apache/iceberg/pull/7641#discussion_r1198962340
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java:
##########
@@ -92,6 +92,18 @@ private TableMetadata loadTableMetadata(String metadataLocation, Reference refer
// Nessie CLI will provide a reference aware GC functionality for the expired/unreferenced
// files.
newProperties.put(TableProperties.GC_ENABLED, "false");
+
+ boolean metadataCleanupEnabled =
+ newProperties
+ .getOrDefault(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "false")
+ .equalsIgnoreCase("true");
+ if (metadataCleanupEnabled) {
+ newProperties.put(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "false");
+ LOG.warn(
+ "Clean up of table metadata files during commit is disabled as it can be in use by other references."
+ + " Use nessie-gc tool for reference aware GC");
Review Comment:
How about `Automatic table metadata files cleanup was requested, but disabled because the Nessie catalog can use historical metadata file. Please set the " + TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED + " property to 'false'. Use the 'nessie-gc' tool for history-aware GC".`?
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java:
##########
@@ -92,6 +92,18 @@ private TableMetadata loadTableMetadata(String metadataLocation, Reference refer
// Nessie CLI will provide a reference aware GC functionality for the expired/unreferenced
// files.
newProperties.put(TableProperties.GC_ENABLED, "false");
+
+ boolean metadataCleanupEnabled =
+ newProperties
+ .getOrDefault(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "false")
+ .equalsIgnoreCase("true");
+ if (metadataCleanupEnabled) {
+ newProperties.put(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "false");
+ LOG.warn(
+ "Clean up of table metadata files during commit is disabled as it can be in use by other references."
+ + " Use nessie-gc tool for reference aware GC");
Review Comment:
I guess the WARN is going to be logged on every load, unless the user permanently resets the `METADATA_DELETE_AFTER_COMMIT_ENABLED` property to `false`, right?
--
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
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #7641: Nessie: Disable table metadata files cleanup during commits
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #7641:
URL: https://github.com/apache/iceberg/pull/7641#discussion_r1198550489
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -579,6 +584,50 @@ public void testGCEnabled() {
"Cannot expire snapshots: GC is disabled (deleting files may corrupt other tables)");
}
+ @Test
+ public void testTableMetadataFilesCleanupDisable() {
+ Table icebergTable = catalog.loadTable(TABLE_IDENTIFIER);
+
+ Assertions.assertThat(
+ icebergTable.properties().get(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED))
+ .isNotNull()
+ .isEqualTo("false");
+
+ // Forceful setting of property also should get override with false
+ icebergTable
+ .updateProperties()
+ .set(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "true")
+ .commit();
+ Assertions.assertThat(
+ icebergTable.properties().get(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED))
+ .isNotNull()
+ .isEqualTo("false");
+
+ icebergTable
+ .updateProperties()
+ .set(TableProperties.METADATA_PREVIOUS_VERSIONS_MAX, "1")
+ .commit();
+
+ String metadataFileLocation =
+ ((BaseTable) icebergTable).operations().current().metadataFileLocation();
+ Path metadataFileLocationPath = Paths.get(metadataFileLocation.replaceFirst("file:", ""));
+
+ Assertions.assertThat(metadataFileLocationPath).exists();
+
+ icebergTable.updateSchema().addColumn("x1", Types.LongType.get()).commit();
+ icebergTable.updateSchema().addColumn("x2", Types.LongType.get()).commit();
+
+ // old table metadata file should still exist after commits.
+ Assertions.assertThat(metadataFileLocationPath).exists();
Review Comment:
Nice suggestion. Added 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-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
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #7641: Nessie: Disable table metadata files cleanup during commits
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #7641:
URL: https://github.com/apache/iceberg/pull/7641#discussion_r1202501759
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java:
##########
@@ -92,6 +92,11 @@ private TableMetadata loadTableMetadata(String metadataLocation, Reference refer
// Nessie CLI will provide a reference aware GC functionality for the expired/unreferenced
// files.
newProperties.put(TableProperties.GC_ENABLED, "false");
+
+ newProperties.put(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "false");
+ LOG.info(
Review Comment:
@snazy: We are waiting for your approval.
--
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
[GitHub] [iceberg] ajantha-bhat commented on pull request #7641: Nessie: Disable table metadata files cleanup during commits
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #7641:
URL: https://github.com/apache/iceberg/pull/7641#issuecomment-1556503948
@snazy, @dimas-b: Addressed the comments. Please take a look at it again.
--
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
[GitHub] [iceberg] ajantha-bhat commented on pull request #7641: Nessie: Disable table metadata files cleanup during commits
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #7641:
URL: https://github.com/apache/iceberg/pull/7641#issuecomment-1552794427
cc: @nastra, @snazy, @dimas-b
Since https://github.com/apache/iceberg/pull/7576 may take more time to conclude (or may not get merged). I have opened a catalog-specific PR targeted for the 1.3.0 release to avoid facing this problem for the users.
--
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
[GitHub] [iceberg] dimas-b commented on a diff in pull request #7641: Nessie: Disable table metadata files cleanup during commits
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #7641:
URL: https://github.com/apache/iceberg/pull/7641#discussion_r1198075839
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -579,6 +584,44 @@ public void testGCEnabled() {
"Cannot expire snapshots: GC is disabled (deleting files may corrupt other tables)");
}
+ @Test
+ public void testTableMetadataFilesCleanupDisable() {
+ Table icebergTable = catalog.loadTable(TABLE_IDENTIFIER);
+
+ Assertions.assertThat(
+ icebergTable.properties().get(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED))
+ .isNotNull()
+ .isEqualTo("false");
+
+ icebergTable
+ .updateProperties()
+ .set(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "true")
+ .commit();
+ icebergTable
+ .updateProperties()
+ .set(TableProperties.METADATA_PREVIOUS_VERSIONS_MAX, "1")
+ .commit();
+
+ String metadataFileLocation =
+ ((BaseTable) icebergTable).operations().current().metadataFileLocation();
+ Path metadataFileLocationPath = Paths.get(metadataFileLocation.replaceFirst("file:", ""));
Review Comment:
If it's a common pattern in test code, I'm fine with following it in Nessie 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: 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
[GitHub] [iceberg] snazy commented on a diff in pull request #7641: Nessie: Disable table metadata files cleanup during commits
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #7641:
URL: https://github.com/apache/iceberg/pull/7641#discussion_r1198659162
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java:
##########
@@ -92,6 +92,11 @@ private TableMetadata loadTableMetadata(String metadataLocation, Reference refer
// Nessie CLI will provide a reference aware GC functionality for the expired/unreferenced
// files.
newProperties.put(TableProperties.GC_ENABLED, "false");
+
+ newProperties.put(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "false");
+ LOG.info(
Review Comment:
This log message is meaningless, nothing actionable, I find it confusing.
--
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
[GitHub] [iceberg] dimas-b commented on a diff in pull request #7641: Nessie: Disable table metadata files cleanup during commits
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #7641:
URL: https://github.com/apache/iceberg/pull/7641#discussion_r1197811718
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -579,6 +584,44 @@ public void testGCEnabled() {
"Cannot expire snapshots: GC is disabled (deleting files may corrupt other tables)");
}
+ @Test
+ public void testTableMetadataFilesCleanupDisable() {
+ Table icebergTable = catalog.loadTable(TABLE_IDENTIFIER);
+
+ Assertions.assertThat(
+ icebergTable.properties().get(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED))
+ .isNotNull()
+ .isEqualTo("false");
+
+ icebergTable
+ .updateProperties()
+ .set(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "true")
Review Comment:
Is this expected to be overridden back to `false` by the Nessie Catalog? If so, shouldn't we assert that?
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -579,6 +584,44 @@ public void testGCEnabled() {
"Cannot expire snapshots: GC is disabled (deleting files may corrupt other tables)");
}
+ @Test
+ public void testTableMetadataFilesCleanupDisable() {
+ Table icebergTable = catalog.loadTable(TABLE_IDENTIFIER);
+
+ Assertions.assertThat(
+ icebergTable.properties().get(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED))
+ .isNotNull()
+ .isEqualTo("false");
+
+ icebergTable
+ .updateProperties()
+ .set(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "true")
+ .commit();
+ icebergTable
+ .updateProperties()
+ .set(TableProperties.METADATA_PREVIOUS_VERSIONS_MAX, "1")
+ .commit();
+
+ String metadataFileLocation =
+ ((BaseTable) icebergTable).operations().current().metadataFileLocation();
+ Path metadataFileLocationPath = Paths.get(metadataFileLocation.replaceFirst("file:", ""));
Review Comment:
Would it be more robust to parse `metadataFileLocation` as a URI, assert schema value, and call `.getPath()`?
--
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
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #7641: Nessie: Disable table metadata files cleanup during commits
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #7641:
URL: https://github.com/apache/iceberg/pull/7641#discussion_r1198664218
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java:
##########
@@ -92,6 +92,11 @@ private TableMetadata loadTableMetadata(String metadataLocation, Reference refer
// Nessie CLI will provide a reference aware GC functionality for the expired/unreferenced
// files.
newProperties.put(TableProperties.GC_ENABLED, "false");
+
+ newProperties.put(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "false");
+ LOG.info(
Review Comment:
If the user has set the table property and might expect it to work. So, checking logs can help in analyzing why it didn't work.
Do you think printing a warning log only when the user has set this property is useful?
--
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
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #7641: Nessie: Disable table metadata files cleanup during commits
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #7641:
URL: https://github.com/apache/iceberg/pull/7641#discussion_r1198975950
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java:
##########
@@ -92,6 +92,18 @@ private TableMetadata loadTableMetadata(String metadataLocation, Reference refer
// Nessie CLI will provide a reference aware GC functionality for the expired/unreferenced
// files.
newProperties.put(TableProperties.GC_ENABLED, "false");
+
+ boolean metadataCleanupEnabled =
+ newProperties
+ .getOrDefault(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "false")
+ .equalsIgnoreCase("true");
+ if (metadataCleanupEnabled) {
+ newProperties.put(TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED, "false");
+ LOG.warn(
+ "Clean up of table metadata files during commit is disabled as it can be in use by other references."
+ + " Use nessie-gc tool for reference aware GC");
Review Comment:
> How about Automatic table metadata files cleanup was requested, but disabled because the Nessie catalog can use historical metadata files. Please set the " + TableProperties.METADATA_DELETE_AFTER_COMMIT_ENABLED + " property to 'false'. Use the 'nessie-gc' tool for history-aware GC".?
I don't think user has to reset it again (based on my above comment). So, I am thinking to update like this.
`Automatic table metadata files cleanup was requested, but disabled because the Nessie catalog can use historical metadata files from other references. Use the 'nessie-gc' tool for history-aware GC".`
--
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
[GitHub] [iceberg] Fokko merged pull request #7641: Nessie: Disable table metadata files cleanup during commits
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #7641:
URL: https://github.com/apache/iceberg/pull/7641
--
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