You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "aokolnychyi (via GitHub)" <gi...@apache.org> on 2023/08/23 23:00:22 UTC
[GitHub] [iceberg] aokolnychyi opened a new pull request, #8383: Spark 3.4: Clarify concurrent table modification error in copy-on-write operations
aokolnychyi opened a new pull request, #8383:
URL: https://github.com/apache/iceberg/pull/8383
Tests that cover concurrent modifications and row-level operations sporadically fail for me locally. The primary reason is that if table caching is disabled, the table would be eagerly refreshed in each scan. That means we can pick up concurrent changes even though they were done via another `Table` instance.
--
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] jerqi commented on a diff in pull request #8383: Spark 3.4: Clarify concurrent table modification error in copy-on-write operations
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #8383:
URL: https://github.com/apache/iceberg/pull/8383#discussion_r1309563218
##########
spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestUpdate.java:
##########
@@ -687,6 +696,13 @@ public synchronized void testUpdateWithSnapshotIsolation()
throws InterruptedException, ExecutionException {
// cannot run tests with concurrency for Hadoop tables without atomic renames
Assume.assumeFalse(catalogName.equalsIgnoreCase("testhadoop"));
+ // if caching is off, the table is eagerly refreshed during runtime filtering
+ // this can cause a validation exception as concurrent changes would be visible
Review Comment:
Should we extract this code to a method in the `SparkRowLevelOperationsTestBase`? If so, we can reduce the duplicated code and comment.
--
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] szehon-ho commented on a diff in pull request #8383: Spark 3.4: Clarify concurrent table modification error in copy-on-write operations
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #8383:
URL: https://github.com/apache/iceberg/pull/8383#discussion_r1310733474
##########
spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestDelete.java:
##########
@@ -948,6 +950,13 @@ public void testDeleteThatRequiresGroupingBeforeWrite() throws NoSuchTableExcept
public synchronized void testDeleteWithSerializableIsolation() throws InterruptedException {
// cannot run tests with concurrency for Hadoop tables without atomic renames
Assume.assumeFalse(catalogName.equalsIgnoreCase("testhadoop"));
+ // if caching is off, the table is eagerly refreshed during runtime filtering
+ // this can cause a validation exception as concurrent changes would be visible
+ Assume.assumeTrue(
+ PropertyUtil.propertyAsBoolean(
+ catalogConfig,
+ CatalogProperties.CACHE_ENABLED,
+ CatalogProperties.CACHE_ENABLED_DEFAULT));
Review Comment:
Nit: how about making this explicitly true?
--
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] aokolnychyi commented on pull request #8383: Spark 3.4: Clarify concurrent table modification error in copy-on-write operations
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #8383:
URL: https://github.com/apache/iceberg/pull/8383#issuecomment-1700144434
Thanks for reviewing, @szehon-ho @jerqi!
--
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] aokolnychyi commented on a diff in pull request #8383: Spark 3.4: Clarify concurrent table modification error in copy-on-write operations
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8383:
URL: https://github.com/apache/iceberg/pull/8383#discussion_r1310789740
##########
spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestUpdate.java:
##########
@@ -687,6 +696,13 @@ public synchronized void testUpdateWithSnapshotIsolation()
throws InterruptedException, ExecutionException {
// cannot run tests with concurrency for Hadoop tables without atomic renames
Assume.assumeFalse(catalogName.equalsIgnoreCase("testhadoop"));
+ // if caching is off, the table is eagerly refreshed during runtime filtering
+ // this can cause a validation exception as concurrent changes would be visible
Review Comment:
I added a common method to check whether caching is enabled, but I feel the comment must be in the test itself for better visibility.
--
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] aokolnychyi commented on a diff in pull request #8383: Spark 3.4: Clarify concurrent table modification error in copy-on-write operations
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8383:
URL: https://github.com/apache/iceberg/pull/8383#discussion_r1303624273
##########
spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/source/SparkCopyOnWriteScan.java:
##########
@@ -107,6 +107,7 @@ public void filter(Filter[] filters) {
Objects.equals(snapshotId(), currentSnapshotId()),
"Runtime file filtering is not possible: the table has been concurrently modified. "
+ "Row-level operation scan snapshot ID: %s, current table snapshot ID: %s. "
+ + "If an external process modifies the table, enable table caching in the catalog. "
Review Comment:
@rdblue, could you review this one? It is related to #6077 you took a look at earlier.
--
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] jerqi commented on a diff in pull request #8383: Spark 3.4: Clarify concurrent table modification error in copy-on-write operations
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #8383:
URL: https://github.com/apache/iceberg/pull/8383#discussion_r1309561183
##########
spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestUpdate.java:
##########
@@ -687,6 +696,13 @@ public synchronized void testUpdateWithSnapshotIsolation()
throws InterruptedException, ExecutionException {
// cannot run tests with concurrency for Hadoop tables without atomic renames
Assume.assumeFalse(catalogName.equalsIgnoreCase("testhadoop"));
+ // if caching is off, the table is eagerly refreshed during runtime filtering
Review Comment:
Should we extract these code to a method in the `SparkRowLevelOperationsTestBase`? If so, we can reduce the duplicated code and comment.
--
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] jerqi commented on a diff in pull request #8383: Spark 3.4: Clarify concurrent table modification error in copy-on-write operations
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #8383:
URL: https://github.com/apache/iceberg/pull/8383#discussion_r1309561183
##########
spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestUpdate.java:
##########
@@ -687,6 +696,13 @@ public synchronized void testUpdateWithSnapshotIsolation()
throws InterruptedException, ExecutionException {
// cannot run tests with concurrency for Hadoop tables without atomic renames
Assume.assumeFalse(catalogName.equalsIgnoreCase("testhadoop"));
+ // if caching is off, the table is eagerly refreshed during runtime filtering
Review Comment:
Should we extract these code to a method in the `SparkRowLevelOperationsTestBase`? If so, we can reduce the duplicated code and comment.
--
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] aokolnychyi merged pull request #8383: Spark 3.4: Clarify concurrent table modification error in copy-on-write operations
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi merged PR #8383:
URL: https://github.com/apache/iceberg/pull/8383
--
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] aokolnychyi commented on a diff in pull request #8383: Spark 3.4: Clarify concurrent table modification error in copy-on-write operations
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8383:
URL: https://github.com/apache/iceberg/pull/8383#discussion_r1310790620
##########
spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestDelete.java:
##########
@@ -948,6 +950,13 @@ public void testDeleteThatRequiresGroupingBeforeWrite() throws NoSuchTableExcept
public synchronized void testDeleteWithSerializableIsolation() throws InterruptedException {
// cannot run tests with concurrency for Hadoop tables without atomic renames
Assume.assumeFalse(catalogName.equalsIgnoreCase("testhadoop"));
+ // if caching is off, the table is eagerly refreshed during runtime filtering
+ // this can cause a validation exception as concurrent changes would be visible
+ Assume.assumeTrue(
+ PropertyUtil.propertyAsBoolean(
+ catalogConfig,
+ CatalogProperties.CACHE_ENABLED,
+ CatalogProperties.CACHE_ENABLED_DEFAULT));
Review Comment:
We have to use the actual default value cause we are simply checking if it is enabled. If we later change the default value, this test should not be affected.
--
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