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

brfrn169 · 2023-11-29T16:50:40Z

Description

This PR disables implicit pre-read by default for Put operations because the feature is backward incompatible.

Related issues and/or PRs

Support implicit pre-read in Consensus Commit #1222

Changes made

Disabled implicit pre-read by default for Put operations
Modified related unit tests and integration tests
Revised related documents

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

N/A

komamitsu · 2023-11-30T04:40:33Z

docs/api-guide.md

+{% capture notice--info %}
+**Note**
+
+When you update an existing record, you need to read the record by using `Get` or `Scan` before using a `Put` operation. Otherwise, the operation will fail due to a conflict. This occurs because ScalarDB assumes that another transaction wrote the record. Instead of reading the record explicitly, you can enable implicit pre-read. See [Enable implicit pre-read for `Put` operations](#enable-implicit-pre-read-for-put-operations) for details.


Otherwise, the operation will fail due to a conflict. This occurs because ScalarDB assumes that another transaction wrote the record

My understanding is as follows

ScalarDB needs to detect if another transaction wrote the record (== conflict)

Put operation without reading the record will fail since ScalarDB can't check the conflict

If it's correct, there might be room to make the description clearer.

I think your understanding is correct. However, since this document is not intended to explain the details of the transaction mechanism in ScalarDB, we might not need to explain it deeply here. Or do you have an idea for this? If yes, can you please suggest it? Thanks.

It might be a bit deep (so it might not be proper for this document), but how about the following description?

Suggested change

When you update an existing record, you need to read the record by using `Get` or `Scan` before using a `Put` operation. Otherwise, the operation will fail due to a conflict. This occurs because ScalarDB assumes that another transaction wrote the record. Instead of reading the record explicitly, you can enable implicit pre-read. See [Enable implicit pre-read for `Put` operations](#enable-implicit-pre-read-for-put-operations) for details.

When you update an existing record, you need to read the record by using `Get` or `Scan` before using a `Put` operation. Otherwise, the operation will fail due to a conflict. ScalarDB needs to check whether another transaction wrote the record for concurrency control when it runs write operations, but ScalarDB cannot do that if you don't read the existing record before write operations. This causes the conflict error. Instead of reading the record explicitly, you can enable implicit pre-read. See [Enable implicit pre-read for `Put` operations](#enable-implicit-pre-read-for-put-operations) for details.

Or, how about the following simple description? I think this is not a deep.

Suggested change

When you update an existing record, you need to read the record by using `Get` or `Scan` before using a `Put` operation. Otherwise, the operation will fail due to a conflict. This occurs because ScalarDB assumes that another transaction wrote the record. Instead of reading the record explicitly, you can enable implicit pre-read. See [Enable implicit pre-read for `Put` operations](#enable-implicit-pre-read-for-put-operations) for details.

When you update an existing record, you need to read the record by using `Get` or `Scan` before using a `Put` operation. Otherwise, the operation will fail due to a conflict. This occurs because the specification of ScalarDB to manage transactions properly. Instead of reading the record explicitly, you can enable implicit pre-read. See [Enable implicit pre-read for `Put` operations](#enable-implicit-pre-read-for-put-operations) for details.

@komamitsu (CC: @feeblefakie) What do you think about @kota2and3kan's idea?

I noticed Enable implicit pre-read for Put operations already has a detailed description about the background. So, the following simple one is okay?

When you update an existing record, you need to read the record by using `Get` or `Scan` before using a `Put` operation. The implicit pre-read feature would be useful for that. See [Enable implicit pre-read for `Put` operations](#enable-implicit-pre-read-for-put-operations) for details.

When you update an existing record, you need to read the record by using Get or Scan before using a Put operation. Otherwise, the operation will fail due to a conflict. This occurs because the specification of ScalarDB to manage transactions properly. Instead of reading the record explicitly, you can enable implicit pre-read. See Enable implicit pre-read for Put operations for details.

This looks simple and users can know the details with Enable implicit pre-read for Put operations.

Only one thing:

This occurs because the specification of ScalarDB to manage transactions properly.

This occurs because of the specification of ScalarDB to manage transactions properly. seems a bit better.

Okay, thank you. Let me fix with This occurs because of the specification of ScalarDB to manage transactions properly. Thanks.

Only one thing:

This occurs because the specification of ScalarDB to manage transactions properly.

This occurs because of the specification of ScalarDB to manage transactions properly. seems a bit better.

Ah, sorry... I overlooked the grammar error... 🙇
I fixed my suggestion just in case!

Suggested change

When you update an existing record, you need to read the record by using `Get` or `Scan` before using a `Put` operation. Otherwise, the operation will fail due to a conflict. This occurs because ScalarDB assumes that another transaction wrote the record. Instead of reading the record explicitly, you can enable implicit pre-read. See [Enable implicit pre-read for `Put` operations](#enable-implicit-pre-read-for-put-operations) for details.

When you update an existing record, you need to read the record by using `Get` or `Scan` before using a `Put` operation. Otherwise, the operation will fail due to a conflict. This occurs because of the specification of ScalarDB to manage transactions properly. Instead of reading the record explicitly, you can enable implicit pre-read. See [Enable implicit pre-read for `Put` operations](#enable-implicit-pre-read-for-put-operations) for details.

Fixed in dfec35a. Thanks.

komamitsu · 2023-11-30T04:53:49Z

docs/api-guide.md

        .build();
 ```

+Note that if you are certain that a record you are trying to mutate does not exist, you should not enable implicit pre-read for the `Put` operation for better performance. For example, if you load initial data, you should not enable implicit pre-read. A `Put` operation without implicit pre-read is faster than `Put` operation with implicit pre-read because the operation skips an unnecessary read.


For example, if you load initial data, you should not enable implicit pre-read.

I think load fits a limited situation. insert a new record or something sounds more general?

I think load initial data is an example of the situation where you are certain that a record you are trying to mutate does not exist. insert a new record sounds too general and it's not an example to me. What do you think?

I see. Your intention sounds reasonable 👍

I just thought "load" sounds to make a target state expected one. Insertions would be mainly used for that, but updates might be used. So, I slightly preferred explicitly mentioning insert. But either sounds okay!

kota2and3kan

LGTM! Thank you!

komamitsu

LGTM! 👍

josh-wong

Looking good! I've added some comments and suggestions. PTAL!

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

docs/api-guide.md

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

brfrn169 · 2023-11-30T07:58:11Z

@josh-wong Thank you for taking at this! I've applied your suggestions. Please take a look again!

josh-wong

LGTM! Thank you!🙇‍♂️

feeblefakie

LGTM! Thank you!

Disable implicit pre-read by default for Put operations

ecd12f3

brfrn169 added the improvement label Nov 29, 2023

brfrn169 self-assigned this Nov 29, 2023

brfrn169 requested review from komamitsu, feeblefakie, Torch3333, josh-wong and kota2and3kan November 29, 2023 16:51

Fix document

06e372a

komamitsu reviewed Nov 30, 2023

View reviewed changes

Revise doc based on feedback

dfec35a

brfrn169 requested a review from komamitsu November 30, 2023 06:23

kota2and3kan approved these changes Nov 30, 2023

View reviewed changes

komamitsu approved these changes Nov 30, 2023

View reviewed changes

josh-wong requested changes Nov 30, 2023

View reviewed changes

Apply suggestions from code review

f51c255

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

brfrn169 requested a review from josh-wong November 30, 2023 07:57

josh-wong approved these changes Nov 30, 2023

View reviewed changes

Merge branch 'master' into disable-implicit-pre-read-by-default-for-put

5b66a2b

feeblefakie approved these changes Dec 1, 2023

View reviewed changes

feeblefakie merged commit f77b0a8 into master Dec 1, 2023
23 checks passed

feeblefakie deleted the disable-implicit-pre-read-by-default-for-put branch December 1, 2023 01:13

feeblefakie pushed a commit that referenced this pull request Dec 1, 2023

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

b347b89

feeblefakie mentioned this pull request Dec 1, 2023

Backport to branch(3) : Disable implicit pre-read by default for Put operations #1339

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

brfrn169 commented Nov 29, 2023

komamitsu Nov 30, 2023

brfrn169 Nov 30, 2023

kota2and3kan Nov 30, 2023

brfrn169 Nov 30, 2023

komamitsu Nov 30, 2023

komamitsu Nov 30, 2023

brfrn169 Nov 30, 2023

kota2and3kan Nov 30, 2023

brfrn169 Nov 30, 2023

komamitsu Nov 30, 2023

brfrn169 Nov 30, 2023 •

edited

Loading

komamitsu Nov 30, 2023

kota2and3kan left a comment

komamitsu left a comment

josh-wong left a comment

brfrn169 commented Nov 30, 2023

josh-wong left a comment

feeblefakie left a comment

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

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

Conversation

brfrn169 commented Nov 29, 2023

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

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 Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kota2and3kan left a comment

Choose a reason for hiding this comment

komamitsu 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 30, 2023

josh-wong left a comment

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

brfrn169 Nov 30, 2023 •

edited

Loading