Support implicit pre-read in Consensus Commit #1222

brfrn169 · 2023-10-23T08:44:21Z

Description

This PR adds support for implicit pre-read in Consensus Commit. With implicit pre-read, if a record is in the write set but not in the read set, ScalarDB will automatically read the record and add it to the read set before preparing records. This allows users to perform mutations (Put and Delete operations) without reading the records beforehand.

One scenario where the pre-read isn't necessary is when writing a record that doesn't already exist. In such a case, users originally don't need to read the record before writing it. To address this, we can disable implicit pre-read for Put operations by calling the Put.disableImplicitPreRead() method. When disabling implicit pre-read, ScalarDB will not read the record automatically before preparing it.

Related issues and/or PRs

N/A

Changes made

Execute implicit pre-read right before preparing records
Also execute implicit pre-read when condition is specified for the Put and Delete operations
Added the the disableImplicitPreRead() method to the Put operation to disable implicit pre-read
Added a new configuration scalar.db.consensus_commit.parallel_implicit_pre_read.enabled that indicates whether executing implicit pre-read in parallel

Checklist

I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation to reflect the changes.
Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
Tests (unit, integration, etc.) have been added for the changes.
My changes generate no new warnings.
Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

N/A

Release notes

Supported implicit pre-read in Consensus Commit. With implicit pre-read, users can perform mutations (Put and Delete operations) without reading the records beforehand.

brfrn169 · 2023-10-23T08:45:29Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommit.java

+    // Fill the read set with records from the write and delete sets if they are unread
+    try {
+      crud.fillReadSetForRecordsFromWriteAndDeleteSetsIfUnread();
+    } catch (CrudConflictException e) {
+      throw new CommitConflictException(
+          "Conflict occurred while reading unread records in the write and delete sets",
+          e,
+          getId());
+    } catch (CrudException e) {
+      throw new CommitException(
+          "Failed to read unread records in the write and delete sets", e, getId());
+    }


Before committing (preparing) a transaction, we will fill the read set with records from the write and delete sets if they are unread.

brfrn169 · 2023-10-23T08:46:16Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java

+    Snapshot.Key key = new Snapshot.Key(put);
+
+    if (put.getCondition().isPresent()) {
+      fillReadSetIfUnread(key);


We will also fill the read set if the mutation has a condition.

brfrn169 · 2023-10-23T08:46:25Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java

+    Snapshot.Key key = new Snapshot.Key(delete);
+
+    if (delete.getCondition().isPresent()) {
+      fillReadSetIfUnread(key);


Ditto. We will also fill the read set if the mutation has a condition.

brfrn169 · 2023-10-23T08:47:36Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java

+      }
+    }
+
+    parallelExecutor.fillReadSetForRecordsFromWriteAndDeleteSetsIfUnread(tasks, snapshot.getId());


Use ParallelExecutor to fill read set in parallel for better performance.

brfrn169 · 2023-10-23T08:48:39Z

docs/configurations.md

+| `scalar.db.consensus_commit.parallel_rollback.enabled`      | Whether or not the rollback phase is executed in parallel.                                          | The value of `scalar.db.consensus_commit.parallel_commit.enabled` |
+| `scalar.db.consensus_commit.async_commit.enabled`           | Whether or not the commit phase is executed asynchronously.                                         | `false`                                                           |
+| `scalar.db.consensus_commit.async_rollback.enabled`         | Whether or not the rollback phase is executed asynchronously.                                       | The value of `scalar.db.consensus_commit.async_commit.enabled`    |
+| `scalar.db.consensus_commit.parallel_fill_read_set.enabled` | Whether or not to fill the read set with records from the write and delete sets if they are unread. | `true`                                                            |


Added a new configuration scalar.db.consensus_commit.parallel_fill_read_set.enabled.

brfrn169 · 2023-10-23T08:50:11Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommit.java

+    // Fill the read set with records from the write and delete sets if they are unread
+    try {
+      crud.fillReadSetForRecordsFromWriteAndDeleteSetsIfUnread();
+    } catch (CrudConflictException e) {
+      throw new PreparationConflictException(
+          "Conflict occurred while reading unread records in the write and delete sets",
+          e,
+          getId());
+    } catch (CrudException e) {
+      throw new PreparationException(
+          "Failed to read unread records in the write and delete sets", e, getId());
+    }


Before preparing a transaction, we will fill the read set with records from the write and delete sets if they are unread.

komamitsu · 2023-10-24T01:50:54Z

@brfrn169 It seems you removed the review requests. But you may want us to review the PR?

brfrn169 · 2023-10-24T02:34:39Z

@brfrn169 It seems you removed the review requests. But you may want us to review the PR?

@komamitsu Thank you for the reminder! 🙇 Yeah, I found some mistakes in the PR and removed the review requests. I fixed the mistakes and requested review again. Pleas take a look when you have time!

komamitsu · 2023-10-24T06:26:32Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java

+      }
+    }
+
+    parallelExecutor.fillReadSetForRecordsFromWriteAndDeleteSetsIfUnread(tasks, snapshot.getId());


Snapshot is annotated with @NotThreadSafe, so calling Snapshot.put() in concurrent might cause an issue?

In this case, it won't cause an issue because the read set is a concurrent map.

Oh, I see. The inconsistency of class-level non-thread-safety and method-level thread-safety is a little bit confusing, but I don't come up with a good straightforward solution...

The inconsistency of class-level non-thread-safety and method-level thread-safety is a little bit confusing

That's true. For now, I left some comments for the methods. Let's refactoring it if we come up with a good solution.

komamitsu · 2023-10-24T07:03:23Z

docs/api-guide.md


-`Delete` is an operation to delete a record specified by a primary key.
+If you're certain that a record you're trying to write doesn't exist, you can use a blind `Put` operation for better performance. A blind `Put` is faster than a regular `Put` as it doesn't read the record beforehand. However, if the record does exist, the operation will fail due to a conflict.


IIUC, the "blind write" feature in this PR is basically implicitly filling read-set if needed so that users don't need to manually read the target record in advance. But com.scalar.db.api.PutBuilder.Buildable#blind() is to skip the implicit read for insert operations. So it sounds a bit confusing to me.

com.scalar.db.api.PutBuilder.Buildable#create(), insert(), skipImplicitRead() or something might be better instead of com.scalar.db.api.PutBuilder.Buildable#blind()?

Good point. In this context, a blind write means preparing a record without having an existing record (in the read set). So when you set the blind option for the Put operation, we skip the implicit read. What are your thoughts on this?

Sounds good if considering only com.scalar.db.api.PutBuilder.Buildable#blind()

When I read the description of this PR, I thought "blind writes" meant implicit filling read-set. So, I still feel some inconsistency between the description and com.scalar.db.api.PutBuilder.Buildable#blind().

This PR adds support for blind writes in Consensus Commit. Specifically, if a record is in the write set but not in the read set, ScalarDB will read the record and add it to the read set before preparing the records internally.

You're right. The term blind has two meanings in this context.

With this change, users can execute Put and Delete operations without pre-reading the target records, which is called blind writes. Also, we can set the blind option for a Put operation to skip the automatic read set filling. I think we can call the former user-perspective blind writes and the latter transaction-manager-perspective blind writes.

Actually, the user-perspective blind writes aren't really blind writes; they're more like virtual blind writes, since the transaction manager actually reads the target records on behalf of the user. Conversely, the transaction-manager-perspective blind writes are the actual blind writes, since the transaction manager writes the records without reading the target records.

Maybe we don't need to modify the option name blind. And should I update the PR description instead? What are your thoughts?

Thanks for the detailed explanation. I think we are on the same page.

I also agree to the ideas of user-perspective blind writes and transaction-manager-perspective blind writes.

Maybe we don't need to modify the option name blind. And should I update the PR description instead?

Maybe it's related to how we want to appeal this feature to users. If we publicly call user-perspective blind writes "blind write", the blind() option might confuse users. Otherwise, the description and release note (With this enhancement, users can execute blind writes without the need to read records prior to writing them.) section should be updated to clearly describe "blind write" means transaction-manager-perspective blind writes (plus "We don't need in-advance reads anymore when you write a fixed value!"?).

I want to hear opinion from @feeblefakie as a product owner as well.

I've updated the description (and the release notes) slightly. Maybe we should also update the PR title but I didn't for now. Can you please check the description and let me know if anything still seems confusing?

It looks good to me!

komamitsu · 2023-10-24T07:24:41Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java

+    Snapshot.Key key = new Snapshot.Key(delete);
+
+    if (delete.getCondition().isPresent()) {
+      fillReadSetIfUnread(key);


QQ: Why only if a condition is set?
This might be a silly idea or I might be missing something, but how about always calling fillReadSetIfUnread() in delete() and put() even if no condition is set so that a commit doesn't need to call fillReadSetForRecordsFromWriteAndDeleteSetsIfUnread()?

Indeed, we can fill the read set anytime before preparing records. So, as you suggested, we could always call fillReadSetIfUnread() in delete() and put(). The benefit of calling fillReadSetForRecordsFromWriteAndDeleteSetsIfUnread() before preparing records is that we can fill the read set in parallel for better performance. So we basically fill the read set right before preparing records, but for conditional updates, we need to fill the read set at that time.

Understood. Sounds reasonable 👍

komamitsu · 2023-10-24T07:34:09Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ParallelExecutor.java

+          true,
+          "fillReadSetForRecordsFromWriteAndDeleteSetsIfUnread",
+          transactionId);
+    } catch (ExecutionException | ValidationConflictException e) {


[minor] The order of exception classes is opposite to the above existing methods

Let me fix this. Thanks.

Ah, the order is consistently ExecutionException, ValidationConflictException, and CrudException. So I don't think we need to fix it. What do you think?

Oops. I confused ExecutionException with CrudException. You're right. Please ignore my comment above.

brfrn169 · 2023-11-02T05:28:44Z

@komamitsu I've discussed with @feeblefakie offline, and we've decided to name this feature implicit pre-read. Accordingly, I've updated the method names and the configuration name, and also revised the PR title and description, and the documentation. Can you please take a look at this PR once again? Sorry for the inconvenience.

BTW, we've also decided not to use the term blind writes because it's confusing for users.

kota2and3kan

LGTM! Thank you!

komamitsu · 2023-11-02T06:57:36Z

docs/api-guide.md


-`Delete` is an operation to delete a record specified by a primary key.
+If you're certain that a record you're trying to write doesn't exist, you can disable implicit pre-read for the `Put` operation for better performance. A `Put` without implicit pre-read is faster than a regular `Put` as it skips the implicit pre-read. However, if the record does exist, the operation will fail due to a conflict.


If you're certain that a record you're trying to write doesn't exist, you can disable implicit pre-read for the Put operation for better performance.

This describes the implicit pre-read feature can be disabled. But there is no explanation in advance about what "implicit pre-read" is. How about adding a short description about "implicit pre-read" mechanism? It may be helpful for those who are interest in ScalarDB performance, but don't know implicit pre-read happens by default.

Thank you for pointing that out. That's right. I've added a short description about "implicit pre-read" in a7c0071. Please take a look! Thanks.

feeblefakie

Thank you for the naming change!
Left additional comments. PTAL!

feeblefakie · 2023-11-02T08:27:15Z

core/src/main/java/com/scalar/db/api/OperationBuilder.java

@@ -281,6 +281,23 @@ interface ClearValues<T> {
    T clearValue(String columnName);
  }

+  interface ImplicitRreReadEnabled<T> {


Suggested change

interface ImplicitRreReadEnabled<T> {

interface ImplicitPreReadEnabled<T> {

Oops... Let me fix it.

Fixed in a5bdbe7. Thanks.

feeblefakie · 2023-11-02T08:27:45Z

core/src/main/java/com/scalar/db/api/OperationBuilder.java

+     * @param implicitRreReadEnabled whether implicit pre-read is enabled or not
+     * @return the operation builder
+     */
+    T implicitRreReadEnabled(boolean implicitRreReadEnabled);


Suggested change

T implicitRreReadEnabled(boolean implicitRreReadEnabled);

T implicitPreReadEnabled(boolean implicitPreReadEnabled);

I'm also wondering if this should be enableImplicitPreRead to make it consistent with the other method.
(In that case, the interface should probably be ImplicitPreRead.)
What do you think?

I've added the implicitPreReadEnabled(boolean implicitPreReadEnabled) method because it can be useful in some cases.

Let's see the following example:

boolean implicitPreRead = ...; Buildable buildable = Put.newBuilder() .namespace("ns") .table("tbl") .partitionKey(partitionKey) .clusteringKey(clusteringKey) .floatValue("c4", 1.23F) .doubleValue("c5", 4.56); if (!implicitPreRead) { buildable.disableImplicitPreRead(); } Put put = buildable.build();

With the implicitPreReadEnabled(boolean implicitPreReadEnabled) method, the above code can be rewritten as follows:

boolean implicitPreRead = ...; Put put = Put.newBuilder() .namespace("ns") .table("tbl") .partitionKey(partitionKey) .clusteringKey(clusteringKey) .floatValue("c4", 1.23F) .doubleValue("c5", 4.56) .implicitPreReadEnabled(implicitPreRead) .build();

So I think we need a method that takes a boolean parameter that toggles the implicit pre-read feature. However, we can discuss the method name (implicitPreReadEnabled) further if needed. Thank you.

feeblefakie · 2023-11-02T08:28:28Z

core/src/main/java/com/scalar/db/api/PutBuilder.java

+    }
+
+    @Override
+    public Buildable implicitRreReadEnabled(boolean implicitRreReadEnabled) {


Suggested change

public Buildable implicitRreReadEnabled(boolean implicitRreReadEnabled) {

public Buildable implicitPreReadEnabled(boolean implicitPreReadEnabled) {

feeblefakie · 2023-11-02T08:32:13Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java

+    snapshot.put(key, delete);
+  }
+
+  public void executeImplicitPreReadIfEnabled() throws CrudException {


Suggested change

public void executeImplicitPreReadIfEnabled() throws CrudException {

public void readIfImplicitPreReadEnabled() throws CrudException {

The above might be more consistent with other methods like readUnread.

Fixed in 460a579. Thanks.

docs/api-guide.md

docs/storage-abstraction.md

Co-authored-by: Hiroyuki Yamada <mogwaing@gmail.com>

komamitsu

LGTM! 👍

feeblefakie

LGTM! Thank you!

brfrn169 · 2023-11-06T06:05:07Z

@josh-wong Can you please take a look at the documentation part of this PR?

josh-wong

Apologies for the delay! I've added a few comments and suggestions. PTAL!🙇🏻‍♂️

docs/api-guide.md

Co-authored-by: Josh Wong <joshua.wong@scalar-labs.com>

josh-wong

LGTM! Thank you!🙇‍♂️

brfrn169 · 2023-11-16T05:19:24Z

@Torch3333 It's been merged, but please take a look when you come back!

brfrn169 · 2023-11-29T05:43:13Z

@Torch3333 It's merged but please take a look when you are back!

brfrn169 added the improvement label Oct 23, 2023

brfrn169 requested review from komamitsu, feeblefakie, Torch3333 and josh-wong October 23, 2023 08:44

brfrn169 self-assigned this Oct 23, 2023

brfrn169 commented Oct 23, 2023

View reviewed changes

brfrn169 removed request for komamitsu, feeblefakie, Torch3333 and josh-wong October 23, 2023 08:50

brfrn169 force-pushed the support-blind-writes-for-consensus-commit branch from 14cdf6e to e51bc13 Compare October 23, 2023 09:08

brfrn169 requested review from Torch3333, josh-wong, komamitsu and feeblefakie and removed request for Torch3333, josh-wong, komamitsu and feeblefakie October 23, 2023 10:15

Support blind writes for Consensus Commit

077335e

brfrn169 force-pushed the support-blind-writes-for-consensus-commit branch from e51bc13 to 077335e Compare October 23, 2023 15:46

brfrn169 requested review from komamitsu, feeblefakie, Torch3333, josh-wong and kota2and3kan October 24, 2023 02:32

komamitsu reviewed Oct 24, 2023

View reviewed changes

brfrn169 requested review from komamitsu and kota2and3kan November 2, 2023 05:28

kota2and3kan approved these changes Nov 2, 2023

View reviewed changes

komamitsu reviewed Nov 2, 2023

View reviewed changes

komamitsu self-requested a review November 2, 2023 06:59

Add brief explanation of implicit pre-read

a7c0071

feeblefakie reviewed Nov 3, 2023

View reviewed changes

brfrn169 and others added 3 commits November 3, 2023 23:11

Apply suggestions from code review

84e26ae

Co-authored-by: Hiroyuki Yamada <mogwaing@gmail.com>

Fix typo

a5bdbe7

Fix based on feedback

460a579

brfrn169 requested a review from feeblefakie November 3, 2023 14:39

komamitsu approved these changes Nov 6, 2023

View reviewed changes

feeblefakie approved these changes Nov 6, 2023

View reviewed changes

josh-wong requested changes Nov 9, 2023

View reviewed changes

docs/api-guide.md Outdated Show resolved Hide resolved

docs/api-guide.md Outdated Show resolved Hide resolved

docs/api-guide.md Show resolved Hide resolved

brfrn169 and others added 2 commits November 9, 2023 14:24

Update docs/api-guide.md

2cfe9ae

Co-authored-by: Josh Wong <joshua.wong@scalar-labs.com>

Update docs/api-guide.md

be6babe

brfrn169 requested a review from josh-wong November 13, 2023 15:20

josh-wong approved these changes Nov 15, 2023

View reviewed changes

Merge branch 'master' into support-blind-writes-for-consensus-commit

7714208

brfrn169 merged commit 0881450 into master Nov 15, 2023
23 checks passed

brfrn169 deleted the support-blind-writes-for-consensus-commit branch November 15, 2023 07:30

feeblefakie mentioned this pull request Nov 15, 2023

Backport to branch(3) failed: Support implicit pre-read in Consensus Commit #1292

Closed

brfrn169 added a commit that referenced this pull request Nov 15, 2023

Support implicit pre-read in Consensus Commit (#1222)

4a400ad

brfrn169 mentioned this pull request Nov 15, 2023

Backport to branch(3) : Support implicit pre-read in Consensus Commit #1293

Merged

brfrn169 mentioned this pull request Nov 29, 2023

Disable implicit pre-read by default for Put operations #1336

Merged

6 tasks

brfrn169 mentioned this pull request Jan 30, 2024

Should perform lazy recovery in implicit pre-read #1476

Merged

6 tasks


		`Delete` is an operation to delete a record specified by a primary key.
		If you're certain that a record you're trying to write doesn't exist, you can use a blind `Put` operation for better performance. A blind `Put` is faster than a regular `Put` as it doesn't read the record beforehand. However, if the record does exist, the operation will fail due to a conflict.

	interface ImplicitRreReadEnabled<T> {
	interface ImplicitPreReadEnabled<T> {

	T implicitRreReadEnabled(boolean implicitRreReadEnabled);
	T implicitPreReadEnabled(boolean implicitPreReadEnabled);

	public Buildable implicitRreReadEnabled(boolean implicitRreReadEnabled) {
	public Buildable implicitPreReadEnabled(boolean implicitPreReadEnabled) {

	public void executeImplicitPreReadIfEnabled() throws CrudException {
	public void readIfImplicitPreReadEnabled() throws CrudException {

Support implicit pre-read in Consensus Commit #1222

Support implicit pre-read in Consensus Commit #1222

Conversation

brfrn169 commented Oct 23, 2023 • edited

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

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

Choose a reason for hiding this comment

komamitsu commented Oct 24, 2023

brfrn169 commented Oct 24, 2023 • edited

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

Choose a reason for hiding this comment

komamitsu Oct 24, 2023 • edited

Choose a reason for hiding this comment

brfrn169 Oct 24, 2023 • edited

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

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

Choose a reason for hiding this comment

brfrn169 commented Nov 2, 2023 • edited

kota2and3kan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feeblefakie left a 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

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

komamitsu left a comment

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

brfrn169 commented Nov 6, 2023

josh-wong left a comment

Choose a reason for hiding this comment

josh-wong left a comment

Choose a reason for hiding this comment

brfrn169 commented Nov 16, 2023

brfrn169 commented Nov 29, 2023

brfrn169 commented Oct 23, 2023 •

edited

brfrn169 commented Oct 24, 2023 •

edited

komamitsu Oct 24, 2023 •

edited

brfrn169 Oct 24, 2023 •

edited

brfrn169 commented Nov 2, 2023 •

edited