Fix concurrent insertions for Iceberg tables #21250

pratyakshsharma · 2023-10-26T16:22:51Z

Description

Number of successful concurrent inserts/updates to an iceberg table is controlled by the table property commit.retry.num-retries which is by default set to 4 in TableProperties.java class in Iceberg. This PR aims to make this configurable.

Motivation and Context

#21251

Impact

Introduction of new user-facing config for iceberg connector: iceberg.commit-retries

Test Plan

Manual and unit tests

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Iceberg Changes
* Add the support to set commit.retry.num-retries table property with table creation to make the number of attempts to make in case of concurrent upserts configurable.

linux-foundation-easycla · 2023-10-26T16:22:55Z

The committers listed above are authorized under a signed CLA.

✅ login: pratyakshsharma / name: Pratyaksh Sharma (cb449d2)

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergConfig.java

agrawalreetika · 2023-10-27T19:54:45Z

@pratyakshsharma Could you please squash the commits and update Iceberg documentation as well for this?

agrawalreetika · 2023-10-27T19:53:44Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergHiveMetadata.java

        propertiesBuilder.put(DEFAULT_FILE_FORMAT, fileFormat.toString());
+        propertiesBuilder.put(COMMIT_NUM_RETRIES, String.valueOf(commitRetries));


Since this is commit retry property for Iceberg, then I think this should be applicable even for Iceberg tables that are created using the Hadoop or Nessie catalog?

yes it should be.

Then let's introduce this property in https://github.com/prestodb/presto/blob/master/presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergNativeMetadata.java#L157 as well.
Since the Table properties for both Metadata classes are the same, we can extract the common part for setting table properties and use it in both

Sure. I will perform simple tests with nessie to verify this works, and then will introduce it here.

Nessie throws below exception when concurrent queries exceed the table config value -

org.apache.iceberg.exceptions.CommitFailedException: Cannot commit: Reference hash is out of date. Update the reference 'main' and try again at org.apache.iceberg.nessie.NessieTableOperations.doCommit(NessieTableOperations.java:167) at org.apache.iceberg.BaseMetastoreTableOperations.commit(BaseMetastoreTableOperations.java:135) at org.apache.iceberg.BaseTransaction.lambda$commitSimpleTransaction$5(BaseTransaction.java:422) at org.apache.iceberg.util.Tasks$Builder.runTaskWithRetry(Tasks.java:413) at org.apache.iceberg.util.Tasks$Builder.runSingleThreaded(Tasks.java:219) at org.apache.iceberg.util.Tasks$Builder.run(Tasks.java:203) at org.apache.iceberg.util.Tasks$Builder.run(Tasks.java:196) at org.apache.iceberg.BaseTransaction.commitSimpleTransaction(BaseTransaction.java:418) at org.apache.iceberg.BaseTransaction.commitTransaction(BaseTransaction.java:302) at com.facebook.presto.iceberg.IcebergAbstractMetadata.finishInsert(IcebergAbstractMetadata.java:278) at com.facebook.presto.spi.connector.classloader.ClassLoaderSafeConnectorMetadata.finishInsert(ClassLoaderSafeConnectorMetadata.java:452) at com.facebook.presto.metadata.MetadataManager.finishInsert(MetadataManager.java:858) at com.facebook.presto.sql.planner.LocalExecutionPlanner.lambda$createTableFinisher$3(LocalExecutionPlanner.java:3392) at com.facebook.presto.operator.TableFinishOperator.getOutput(TableFinishOperator.java:289) at com.facebook.presto.operator.Driver.processInternal(Driver.java:428) at com.facebook.presto.operator.Driver.lambda$processFor$9(Driver.java:311) at com.facebook.presto.operator.Driver.tryWithLock(Driver.java:732) at com.facebook.presto.operator.Driver.processFor(Driver.java:304) at com.facebook.presto.execution.SqlTaskExecution$DriverSplitRunner.processFor(SqlTaskExecution.java:1079) at com.facebook.presto.execution.executor.PrioritizedSplitRunner.process(PrioritizedSplitRunner.java:165) at com.facebook.presto.execution.executor.TaskExecutor$TaskRunner.run(TaskExecutor.java:621) at com.facebook.presto.$gen.Presto_null__testversion____20231108_094807_1.run(Unknown Source) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) at java.lang.Thread.run(Thread.java:750) Caused by: org.projectnessie.error.NessieReferenceConflictException: Values of existing and expected content for key 'iceberg_db.test_nessie_iceberg' are different. at org.projectnessie.error.ErrorCode.lambda$asException$1(ErrorCode.java:66) at java.util.Optional.map(Optional.java:215) at org.projectnessie.error.ErrorCode.asException(ErrorCode.java:66) at org.projectnessie.client.rest.ResponseCheckFilter.checkResponse(ResponseCheckFilter.java:67) at org.projectnessie.client.rest.NessieHttpResponseFilter.filter(NessieHttpResponseFilter.java:29) at org.projectnessie.client.http.impl.jdk8.UrlConnectionRequest.lambda$executeRequest$2(UrlConnectionRequest.java:86) at java.util.Collections$SingletonList.forEach(Collections.java:4824) at org.projectnessie.client.http.impl.jdk8.UrlConnectionRequest.executeRequest(UrlConnectionRequest.java:86) at org.projectnessie.client.http.impl.jdk8.UrlConnectionRequest.post(UrlConnectionRequest.java:120) at org.projectnessie.client.http.HttpTreeClient.commitMultipleOperations(HttpTreeClient.java:202) at sun.reflect.GeneratedMethodAccessor1216.invoke(Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.projectnessie.client.http.NessieHttpClient$ExceptionRewriter.invoke(NessieHttpClient.java:79) at com.sun.proxy.$Proxy522.commitMultipleOperations(Unknown Source) at org.projectnessie.client.http.v1api.HttpCommitMultipleOperations.commit(HttpCommitMultipleOperations.java:35) at org.apache.iceberg.nessie.NessieIcebergClient.commitTable(NessieIcebergClient.java:479) at org.apache.iceberg.nessie.NessieTableOperations.doCommit(NessieTableOperations.java:160) ... 24 more

Nessie throws below exception when concurrent queries exceed the table config value

Does it work if concurrent queries not exceeding the table config value?

yes, have added a test case for the same.

github-actions · 2023-10-30T06:59:39Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 1a4e945...cb449d2.

Notify	File(s)
@steveburnett	presto-docs/src/main/sphinx/connector/iceberg.rst

pratyakshsharma · 2023-10-30T09:57:18Z

the worker seems to crash as part of the failures in linux-presto-e2e-tests. This happened second time consecutively in this PR.
cc @tdcmeehan @wanglinsong

tdcmeehan · 2023-10-30T13:31:31Z

the worker seems to crash as part of the failures in linux-presto-e2e-tests. This happened second time consecutively in this PR. cc @tdcmeehan @wanglinsong

Can you please ask about this in the #presto-cpp channel in Slack?

steveburnett

LGTM! (docs)

agrawalreetika · 2023-11-09T11:13:13Z

@pratyakshsharma Thanks for verifying it for Nessie as well.
Few things,

Let's modify the Tests in https://github.com/prestodb/presto/blob/master/presto-iceberg/src/test/java/com/facebook/presto/iceberg/hive/TestIcebergSmokeHive.java#L59 with concurrency > 5 (Since 5 is the success case) and add for both the failure and success scenarios there. And if possible we can add similar tests for Nessie as well
I think we can modify the commit message to something like "Introduce iceberg table property commit_retries to handle concurrent insertions"?

pratyakshsharma · 2023-11-09T11:17:24Z

@agrawalreetika yes already working on fixing test cases. Sure will update the commit message.

pratyakshsharma · 2023-11-15T08:56:41Z

I have commented the test case covering failure scenario for concurrent insertions, since it is failing and I am working on fixing it. I have added the expectedExceptions tag, but the test is failing despite that exception getting thrown.

pratyakshsharma · 2023-12-26T06:44:34Z

@yingsu00 As per our internal discussions, I checked JMeter codebase where they spawn new threads for load testing. They use ThreadPoolExecutor for managing the test plan, which in turn starts the new threads as per the configurations provided. ThreadPoolExecutor is just an implementation of ExecutorService. I tried following the same approach for my test case, but surprisingly it did not help.
We can get on a quick call to discuss more.

tdcmeehan · 2024-01-05T20:30:31Z

presto-iceberg/src/test/java/com/facebook/presto/iceberg/nessie/TestIcebergSmokeNessie.java

+        final CountDownLatch countDownLatch = new CountDownLatch(commitRetries);
+        AtomicInteger value = new AtomicInteger(0);
+        Set<Throwable> errors = new CopyOnWriteArraySet<>();
+        List<Thread> threads = Stream.generate(() -> new Thread(() -> {


There are too many tests that try to accomplish similar tasks through multi-threading. Ultimately, these style of tests worry me because they end up being highly environment-specific--suspectible to GC pauses, noisy neighbors and such. Can we think of a different way to accomplish this that isn't time depenendent?

I am currently working on this. Lets see if I am able to come up with something.

I would recommend just removing the test, since this is a table property and a functionality of the core Iceberg library. Adding a potentially flaky test to our suites is just not worth the potential problems.

@tdcmeehan So with the current approach of submitting 100 queries, the test case is able to pass consistently. I am now only trying to fine tune the number of threads and other parameters, so as to reduce the test run time and not introduce flakiness overall.
Let me know if you still think otherwise. We can probably discuss internally on this.
cc @agrawalreetika @imjalpreet @yingsu00 @nmahadevuni

pratyakshsharma · 2024-02-05T12:07:27Z

@yingsu00 @agrawalreetika This is good to review now.

agrawalreetika · 2024-02-05T13:13:36Z

.github/workflows/tests.yml

@@ -45,7 +45,7 @@ jobs:
          - ":presto-tests -P ci-only-distributed-queries"
          - ":presto-tests -P ci-only-aggregation-queries"
          - ":presto-tests -P ci-only-plan-determinism"
-          - ":presto-tests -P ci-only-resource-manager" 
+          - ":presto-tests -P ci-only-resource-manager"


Change is not related?

@pratyakshsharma Can you please remove this change? It seems unrelated.

steveburnett · 2024-02-05T15:12:34Z

Consider changing the release note entry in the Description. Following the release note guidelines, would something like this be a better description of this PR?

== RELEASE NOTES ==

Iceberg Connector Changes
* Improve the commit.retry.num-retries table property to make configurable the number of attempts to make in case of concurrent upserts.

yingsu00

@tdcmeehan The test was removed. Do you want to review this PR again? Thanks!

yingsu00 · 2024-02-06T08:56:51Z

.github/workflows/tests.yml

@@ -45,7 +45,7 @@ jobs:
          - ":presto-tests -P ci-only-distributed-queries"
          - ":presto-tests -P ci-only-aggregation-queries"
          - ":presto-tests -P ci-only-plan-determinism"
-          - ":presto-tests -P ci-only-resource-manager" 
+          - ":presto-tests -P ci-only-resource-manager"


@pratyakshsharma Can you please remove this change? It seems unrelated.

pratyakshsharma · 2024-02-08T07:37:13Z

@steveburnett

So we are not improving the support, rather adding it from scratch in presto-iceberg connector. Please let me know in case of any more queries.

pratyakshsharma · 2024-02-08T12:55:17Z

@yingsu00 This is ready for another pass

steveburnett · 2024-02-08T14:35:35Z

@steveburnett

So we are not improving the support, rather adding it from scratch in presto-iceberg connector. Please let me know in case of any more queries.

Thanks for the explanation! The release note guidelines Order of Changes section suggests "Additions: line starts with Add...". Maybe this suggestion works better?

== RELEASE NOTES ==

Iceberg Connector Changes
* Add the commit.retry.num-retries table property to make configurable the number of attempts to make in case of concurrent upserts.

tdcmeehan · 2024-02-08T14:36:50Z

Agreed with @steveburnett, this is a major change that users of Iceberg connector will be interested in and may want to use. We should add a release note for it.

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergTableProperties.java

tdcmeehan · 2024-02-08T14:37:57Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergTableProperties.java

@@ -73,6 +76,11 @@ public IcebergTableProperties(IcebergConfig icebergConfig)
                        "Format version for the table",
                        null,
                        false))
+                .add(integerProperty(
+                        CONCURRENCY_RETRIES,
+                        "Determines the number of attempts in case of concurrent upserts",


Concurrent updates

I think it is valid in case of inserts as well. Ultimately this boils down to some writer finding the metadata file updated, as compared to what it saw in the beginning, when it tries to commit. So upserts seems more suitable

By update, I am trying to convey a more general meaning that upset, which may include deletes. Feel free to use any language that encapsulates this, but I feel upsert does not.

I can probably make it upserts and deletes then.

pratyakshsharma · 2024-02-08T16:50:30Z

Updated release notes @steveburnett @tdcmeehan

steveburnett · 2024-02-08T17:03:46Z

Updated release notes @steveburnett @tdcmeehan

@pratyakshsharma, it looks great! Thank you.

pratyakshsharma · 2024-02-09T07:04:25Z

The singlestore test failure is unrelated. Raised an issue here - #21892

pratyakshsharma · 2024-02-09T07:26:49Z

This is good for final pass @yingsu00

yingsu00 · 2024-02-13T14:43:02Z

@tdcmeehan Do you have more comments?

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergTableProperties.java

Introduce new iceberg table property `commit_retries` to handle concurrent insertions across catalogs.

pratyakshsharma · 2024-02-14T11:01:13Z

@tdcmeehan please take another pass

pratyakshsharma · 2024-02-14T13:20:49Z

@yingsu00 this is good to merge.

pratyakshsharma requested a review from a team as a code owner October 26, 2023 16:22

pratyakshsharma requested a review from presto-oss October 26, 2023 16:22

pratyakshsharma mentioned this pull request Oct 26, 2023

More than 5 concurrent inserts fail via Iceberg connector #21251

Closed

pratyakshsharma force-pushed the iceberg-concurrency branch 2 times, most recently from e6456d6 to 42e97e8 Compare October 26, 2023 17:40

agrawalreetika reviewed Oct 27, 2023

View reviewed changes

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergConfig.java Outdated Show resolved Hide resolved

agrawalreetika requested changes Oct 27, 2023

View reviewed changes

pratyakshsharma force-pushed the iceberg-concurrency branch from d2bacf8 to 8d53d7d Compare October 30, 2023 06:58

steveburnett approved these changes Oct 30, 2023

View reviewed changes

pratyakshsharma force-pushed the iceberg-concurrency branch 2 times, most recently from 8461a75 to 05615f6 Compare November 9, 2023 07:40

pratyakshsharma changed the title ~~Fix concurrent insertions for hive and glue metastores~~ Fix concurrent insertions for Iceberg tables Nov 9, 2023

pratyakshsharma force-pushed the iceberg-concurrency branch from 05615f6 to 7ad3425 Compare November 15, 2023 08:47

pratyakshsharma mentioned this pull request Dec 27, 2023

Add support for iceberg set properties #21606

Closed

6 tasks

yingsu00 requested a review from tdcmeehan January 4, 2024 05:33

tdcmeehan requested changes Jan 5, 2024

View reviewed changes

pratyakshsharma mentioned this pull request Jan 6, 2024

Add ALTER TABLE SET PROPERTIES statement #21495

Merged

6 tasks

pratyakshsharma force-pushed the iceberg-concurrency branch 4 times, most recently from 4997d80 to d0b8e63 Compare January 26, 2024 08:16

pratyakshsharma force-pushed the iceberg-concurrency branch from 98b9024 to 32b7c1f Compare February 5, 2024 11:14

agrawalreetika reviewed Feb 5, 2024

View reviewed changes

agrawalreetika approved these changes Feb 5, 2024

View reviewed changes

yingsu00 approved these changes Feb 6, 2024

View reviewed changes

pratyakshsharma force-pushed the iceberg-concurrency branch from 32b7c1f to 6c7e025 Compare February 8, 2024 07:34

tdcmeehan reviewed Feb 8, 2024

View reviewed changes

pratyakshsharma force-pushed the iceberg-concurrency branch from f2030b8 to d0fdec1 Compare February 8, 2024 17:02

pratyakshsharma force-pushed the iceberg-concurrency branch from d0fdec1 to 3cac4d1 Compare February 9, 2024 06:40

pratyakshsharma mentioned this pull request Feb 9, 2024

Singlestore tests fail with "Not enough disk space" #21892

Closed

pratyakshsharma force-pushed the iceberg-concurrency branch 2 times, most recently from 0493689 to 35fad55 Compare February 13, 2024 12:12

tdcmeehan reviewed Feb 13, 2024

View reviewed changes

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergTableProperties.java Outdated Show resolved Hide resolved

Add support for iceberg concurrent insertions

cb449d2

Introduce new iceberg table property `commit_retries` to handle concurrent insertions across catalogs.

pratyakshsharma force-pushed the iceberg-concurrency branch from 35fad55 to cb449d2 Compare February 14, 2024 10:11

tdcmeehan approved these changes Feb 14, 2024

View reviewed changes

yingsu00 merged commit e579a3f into prestodb:master Feb 14, 2024
57 checks passed

wanglinsong mentioned this pull request May 1, 2024

Add release notes for 0.287 #22647

Merged

48 tasks

		propertiesBuilder.put(DEFAULT_FILE_FORMAT, fileFormat.toString());
		propertiesBuilder.put(COMMIT_NUM_RETRIES, String.valueOf(commitRetries));

Fix concurrent insertions for Iceberg tables #21250

Fix concurrent insertions for Iceberg tables #21250

Conversation

pratyakshsharma commented Oct 26, 2023 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

linux-foundation-easycla bot commented Oct 26, 2023 • edited Loading

agrawalreetika commented Oct 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawalreetika Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 30, 2023 • edited Loading

pratyakshsharma commented Oct 30, 2023 • edited Loading

tdcmeehan commented Oct 30, 2023

steveburnett left a comment

Choose a reason for hiding this comment

agrawalreetika commented Nov 9, 2023

pratyakshsharma commented Nov 9, 2023

pratyakshsharma commented Nov 15, 2023

pratyakshsharma commented Dec 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pratyakshsharma commented Feb 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveburnett commented Feb 5, 2024

yingsu00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pratyakshsharma commented Feb 8, 2024

pratyakshsharma commented Feb 8, 2024

steveburnett commented Feb 8, 2024

tdcmeehan commented Feb 8, 2024

Choose a reason for hiding this comment

pratyakshsharma Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pratyakshsharma commented Feb 8, 2024

steveburnett commented Feb 8, 2024

pratyakshsharma commented Feb 9, 2024

pratyakshsharma commented Feb 9, 2024

yingsu00 commented Feb 13, 2024

pratyakshsharma commented Feb 14, 2024

pratyakshsharma commented Feb 14, 2024

pratyakshsharma commented Oct 26, 2023 •

edited

Loading

linux-foundation-easycla bot commented Oct 26, 2023 •

edited

Loading

agrawalreetika Oct 30, 2023 •

edited

Loading

github-actions bot commented Oct 30, 2023 •

edited

Loading

pratyakshsharma commented Oct 30, 2023 •

edited

Loading

pratyakshsharma Feb 8, 2024 •

edited

Loading