Support condition for transactional operations #899

Torch3333 · 2023-06-07T06:35:19Z

Context
Setting conditions for Put or Delete in a transaction was not supported. Setting conditions in the operation builder was possible, but they were ignored.

Changes
Setting condition is now fully supported on Put and Delete operation for consensus commit (regular and 2pc) or Jdbc transactions. For example :

scalardb/docs/api-guide.md

Lines 622 to 640 in 46441d2

    
           // Build a condition 
        
           MutationCondition condition = 
        
               ConditionBuilder.putIf(ConditionBuilder.column("c4").isEqualToFloat(0.0F)) 
        
                   .and(ConditionBuilder.column("c5").isEqualToDouble(0.0)) 
        
                   .build(); 
        
           Put put = 
        
               Put.newBuilder() 
        
                   .namespace("ns") 
        
                   .table("tbl") 
        
                   .partitionKey(partitionKey) 
        
                   .clusteringKey(clusteringKey) 
        
                   .floatValue("c4", 1.23F) 
        
                   .doubleValue("c5", 4.56) 
        
                   .condition(condition) // condition 
        
                   .build(); 
        
           // Execute the Put operation 
        
           transaction.put(put);

When the mutation condition is not met, a UnsatisfiedConditionException is thrown when the mutation is executed.

…on_for_transaction # Conflicts: # core/src/main/java/com/scalar/db/transaction/consensuscommit/PrepareMutationComposer.java # core/src/test/java/com/scalar/db/transaction/consensuscommit/PrepareMutationComposerTest.java

…ConflictException instead of CrudException

…on_for_transaction

…onditionException, respectively for regular transaction and 2pc, when the mutation condition is not satisfied

.../java/com/scalar/db/transaction/consensuscommit/ConsensusCommitMutationOperationChecker.java

core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransaction.java

Torch3333 · 2023-06-08T05:56:10Z

rpc/src/main/proto/scalardb.proto

@@ -380,6 +380,7 @@ message TransactionResponse {
      TRANSACTION_CONFLICT = 1;
      UNKNOWN_TRANSACTION_STATUS = 2;
      OTHER = 3;
+      UNSATISFIED_CONDITION = 4;


For the transaction gRPC API, I added a new error code to handle a failed mutation condition.

Torch3333 · 2023-06-08T05:56:48Z

rpc/src/main/proto/scalardb.proto

@@ -515,6 +516,7 @@ message TwoPhaseCommitTransactionResponse {
      TRANSACTION_CONFLICT = 1;
      UNKNOWN_TRANSACTION_STATUS = 2;
      OTHER = 3;
+      UNSATISFIED_CONDITION = 4;


Likewise for the 2PC transaction gRPC API.

Torch3333 · 2023-06-08T05:58:29Z

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

+/**
+ * This class verifies the mutation condition for Put and Delete operation are met by asserting the
+ * condition on the existing record (if any) targeted by the mutation. It does not validate by the
+ * condition by using the underlying storage.
+ */
+@ThreadSafe
+public class ConditionalMutationValidator {


This is the core part of the changes. This class verifies the mutation condition is satisfied without leveraging the underlying storage.

Torch3333 · 2023-06-08T06:04:55Z

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

+      if (composer instanceof PrepareMutationComposer) {
+        conditionalMutationValidator.validateConditionIsSatisfied(entry.getValue(), result);
+      }
      composer.add(entry.getValue(), result);
    }
    for (Entry<Key, Delete> entry : deleteSet.entrySet()) {
      TransactionResult result =
          readSet.containsKey(entry.getKey()) ? readSet.get(entry.getKey()).orElse(null) : null;
+      if (composer instanceof PrepareMutationComposer) {
+        conditionalMutationValidator.validateConditionIsSatisfied(entry.getValue(), result);
+      }


Checking if the mutation condition is met is performed here.

Ideally, I would have preferred to add this call directly to the PrepareMutationComposer class but this means I would have needed to update the MutationComposer.add() interface method signature to throw a PreparationUnsatisfiedConditionException, which seemed more problematic.

Maybe, we can add a new method to validate conditions in mutations in the write set and the delete set to the Snapshot class, and we can call it in CommitHandler.prepare(). What do you think?

Thank you for the suggestion, I agree.
I did as you say and created the Snapshot.validateConditionalMutations() method that is executed from CommitHandler.prepare()
see afc2fbe

Torch3333 · 2023-06-08T06:09:41Z

core/src/main/java/com/scalar/db/common/checker/ConditionChecker.java

+  public static class ConditionCheckerFactory {
+    public ConditionChecker create(TableMetadata tableMetadata) {
+      return new ConditionChecker(tableMetadata);
+    }
+
+    public ConditionCheckerFactory() {}
+  }


I added this to facilitate mocking this class when for the ConsensusCommitMutationOperationChecker unit test.

brfrn169

Overall LGTM. Left one minor comment. Please take a look when you have time!

Since this change affects the core transaction logic, I'll review it again later. Thanks.

.../java/com/scalar/db/transaction/consensuscommit/ConsensusCommitMutationOperationChecker.java

core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransaction.java

komamitsu · 2023-06-13T03:13:15Z

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

+      case NE:
+        return Ordering.natural().compare(existingRecordColumn, conditionalExpressionColumn) != 0;
+        // For 'greater than' and 'less than' types of conditions and when the existing record is
+        // null, we consider the condition to be unsatisfied. This mimic the behavior as if


Suggested change

// null, we consider the condition to be unsatisfied. This mimic the behavior as if

// null, we consider the condition to be unsatisfied. This mimics the behavior as if

Thanks, see 562ca5f

core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransaction.java

komamitsu · 2023-06-13T05:34:13Z

For regular transaction, when a mutation operation condition is not met, committing the transaction will throw a CommitUnsatisfiedConditionException.
For 2pc transaction, when a mutation operation condition is not met, preparing the transaction will throw a PreparationUnsatisfiedConditionException.

@Torch3333 What is the main benefit of having the two separate exceptions? The background of this question is that I'm wondering if it's an option to merge them to just a UnsatisfiedConditionException.

komamitsu

LGTM! 👍

Torch3333 · 2023-06-13T05:55:53Z

@komamitsu
Thanks for the question; I should have detailed the reasoning beforehand.

transaction.commit() and twoPCTransaction.prepare() do not throw a type of exception that is common between them.
commit throws CommitException and UnknownTransactionStatusException while prepare throws PreparationException.
So if we wanted to have a shared UnsatisfiedConditionException for these two methods, we would have needed to add this new exception type to the currently thrown exception.
We did not think it was worth adding a common exception type considering the trouble it would cause our users to have to handle a new checked exception type in their existing codebase.

brfrn169

Left several minor comments. Please take a look when you have time!

brfrn169 · 2023-06-13T05:48:39Z

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

    PrepareMutationComposer composer =
        new PrepareMutationComposer(snapshot.getId(), tableMetadataManager);
+    snapshot.validateConditionalMutations();


I think snapshot.validateConditionalMutations() is no longer a part of prepareRecords(). So can we move it into the prepare() method as follows?

public void prepare(Snapshot snapshot) throws PreparationException { snapshot.validateConditionalMutations(); try { prepareRecords(snapshot); } catch (NoMutationException e) { throw new PreparationConflictException("preparing record exists", e, snapshot.getId()); } catch (RetriableExecutionException e) { throw new PreparationConflictException( "conflict happened when preparing records", e, snapshot.getId()); } catch (ExecutionException e) { throw new PreparationException("preparing records failed", e, snapshot.getId()); } }

What do you think?

Moreover, I'm also wondering if validating conditions should be Snapshot's responsibility or not.

@brfrn169 What do you think?

@feeblefakie That's a good point.

Indeed, from the impression of the name, it doesn't seem like validating conditions is the responsibility of Snapshot. However, if we were to move validateConditionalMutations() into another class, we would need to expose readSet, writeSet, and deleteSet from Snapshot (by adding getter methods for them). And if we proceed with this, it feels like we should also move toSerializableWithExtraWrite() and toSerializableWithExtraRead() into another class. But such changes amount to substantial refactoring, which shouldn't be done in this PR. Maybe we can retain validateConditionalMutations() in Snapshot for now and consider refactoring it in the future.

What do you think?

@brfrn169 Sorry, I should have elaborated more.
I meant, can we check the conditions of mutation operations when calling the operations, not when committing
the transaction? (so that we can do an early abort)
For example, it seems like we can check the conditions of Put when calling CrudHandler.put.
I might miss something, though.

For example, it seems like we can check the conditions of Put when calling CrudHandler.put.

@feeblefakie Ah, indeed, it seems we could do that.

@Torch3333 What do you think?

@brfrn169
After discussing with @feeblefakie this morning, we concluded it is possible to move the condition check to CrudHandler.put and CrudHandler.delete.
I will need to add a getter method in the Snapshot object to read a value from the read set.
Regarding the throw exception when the condition is not satisfied, we can then have a single exception type for regular and 2pc transaction interface that is called UnsatisfiedConditionException (extending CrudException) when executing transaction.put() or transaction.delete().
Overall, it will simplify the implementation.

I revised the PR as described above.
Thank you for the suggestion.

docs/api-guide.md

docs/two-phase-commit-transactions.md

server/src/test/java/com/scalar/db/server/DistributedTransactionServiceTest.java

…on_for_transaction_2 # Conflicts: # docs/api-guide.md

komamitsu

👍

feeblefakie

Overall, looking good. Thank you!
I made some comments and suggestions.
PTAL!
Since it's the core part, I will take another look later.

feeblefakie · 2023-06-14T06:01:31Z

docs/api-guide.md

@@ -583,6 +583,47 @@ Put put =
        .build();
 ```

+##### Put with a condition


@josh-wong Can you review the updates in this guide?

feeblefakie · 2023-06-14T06:23:19Z

docs/api-guide.md

@@ -583,6 +583,47 @@ Put put =
        .build();
 ```

+##### Put with a condition
+


I think it might be helpful to add something like this here.

You can write arbitrary conditions (e.g., a bank account balance must be equal to or more than zero) that you require a transaction to meet before being committed by having some checking logic in the transaction. Alternatively, you can also write simple conditions in a mutation operation, such as Put and Delete.

(If this is added, I think we need to update the next sentence a bit.)

Thank you for the suggestion,
To avoid redundancy in the explanation, I will merge the Put with a condition and Delete with a condition paragraphs.

Fixed in 46441d2

feeblefakie · 2023-06-14T06:35:17Z

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

+   * @param existingRecord the current value of the record targeted by the mutation, if any
+   * @throws PreparationUnsatisfiedConditionException if the condition is not satisfied
+   */
+  public void validateConditionIsSatisfied(Put put, @Nullable TransactionResult existingRecord)


It's a bit picky, but I feel the method name is a little redundant since a condition is valid if a condition is satisfied in this case.
How about the following names?

validateCondition

checkIfConditionIsSatisfied

What do you think?

I see, thank you.
I used a verbose name for this method to be clear that this method is not about checking the condition is not contextually correct (e.g. the column used in the condition matches the table metadata)
So, I will use your second suggestion checkIfConditionIsSatisfied

If you choose checkIfConditionIsSatisfied, then we should probably rename validateConditionalMutations for consistency and clarity.

I renamed the method to MutationConditionsValidator.checkIfConditionIsSatisfied in 46441d2
Thank you.

feeblefakie

Left some additional comments.

feeblefakie · 2023-06-14T09:30:32Z

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

+   * @param existingRecord the current value of the record targeted by the mutation, if any
+   * @throws PreparationUnsatisfiedConditionException if the condition is not satisfied
+   */
+  public void validateConditionIsSatisfied(Put put, @Nullable TransactionResult existingRecord)


If you choose checkIfConditionIsSatisfied, then we should probably rename validateConditionalMutations for consistency and clarity.

feeblefakie · 2023-06-14T10:14:34Z

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

+  }
+
+  /**
+   * This validates the condition for the Put operation is satisfied.


Suggested change

* This validates the condition for the Put operation is satisfied.

* This checks if the condition of the specified Put operation is satisfied for the specified record.

Thank you. It's fixed in 46441d2

feeblefakie · 2023-06-14T10:15:09Z

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

+  }
+
+  /**
+   * This validates the condition for the Delete operation is satisfied.


Thank you. It's fixed in 46441d2

feeblefakie · 2023-06-14T10:21:49Z

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

+import javax.annotation.concurrent.ThreadSafe;
+
+/**
+ * This class verifies the mutation condition for Put and Delete operation are met by asserting the


How about something like this?

This class checks if a record satisfies the conditions of Put and Delete operations that mutate the record.

I feel the sentence It does not leverage the underlying storage to validate the condition. is a bit confusing.

This sounds good, thank you.

Thank you. It's fixed in 46441d2

feeblefakie · 2023-06-14T10:23:48Z

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

+ * underlying storage to validate the condition.
+ */
+@ThreadSafe
+public class ConditionalMutationValidator {


Suggested change

public class ConditionalMutationValidator {

public class MutationConditionsValidator {

I think it validates conditions (not mutations), so should it be like this?

Indeed, thank you.

It's fixed in 46441d2

feeblefakie · 2023-06-14T10:27:12Z

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

    PrepareMutationComposer composer =
        new PrepareMutationComposer(snapshot.getId(), tableMetadataManager);
+    snapshot.validateConditionalMutations();


Moreover, I'm also wondering if validating conditions should be Snapshot's responsibility or not.

@brfrn169 What do you think?

Torch3333 · 2023-06-19T06:46:19Z

@josh-wong I finished revising the documentation, so you can have a look when you get the chance. Thank you.

brfrn169

LGTM! Thank you!

josh-wong

I added a few comments and suggested revisions. PTAL!

docs/api-guide.md

…on_for_transaction_2 # Conflicts: # core/src/test/java/com/scalar/db/transaction/consensuscommit/CrudHandlerTest.java

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

Torch3333 · 2023-06-20T11:09:51Z

@josh-wong Thank you for the suggestions. I applied them all.

josh-wong

LGTM! Thank you!🙇🏻‍♂️

feeblefakie

I left one very minor comment, but LGTM! Thank you!

feeblefakie · 2023-06-21T11:21:54Z

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

@@ -519,6 +519,10 @@ public boolean isValidationRequired() {
    return isExtraReadEnabled();
  }

+  public Optional<TransactionResult> getFromReadSet(Key key) {


Super minor, but I think it's better to be placed below containsKeyInReadSet just to cluster similar methods.

Indeed, thank you. I updated it in c612740

josh-wong

LGTM! Thank you!🙇🏻‍♂️

…scalardb into condition_for_transaction_2

Vincent Guilpain added 5 commits April 28, 2023 11:25

Process conditions for transaction operations

87d8b7a

Add other tests

bdf51bd

For Jdbc transaction, when the condition is not satisfied, throw Crud…

e929182

…ConflictException instead of CrudException

Merge branch 'master' of github.com:scalar-labs/scalardb into conditi…

f78df88

…on_for_transaction

Torch3333 force-pushed the condition_for_transaction_2 branch from c0484d3 to db8b3fe Compare June 8, 2023 05:08

Throw CommitUnsatisfiedConditionException and PreparationUnsatisfiedC…

5ab9a0c

…onditionException, respectively for regular transaction and 2pc, when the mutation condition is not satisfied

Torch3333 force-pushed the condition_for_transaction_2 branch from db8b3fe to 5ab9a0c Compare June 8, 2023 07:15

Torch3333 changed the title ~~Support conditions for transactional operations~~ Support condition for transactional operations Jun 8, 2023

Torch3333 marked this pull request as ready for review June 8, 2023 08:09

Torch3333 self-assigned this Jun 8, 2023

Torch3333 added the enhancement New feature or request label Jun 8, 2023

Torch3333 requested review from komamitsu, brfrn169 and feeblefakie June 8, 2023 08:10

Torch3333 commented Jun 8, 2023

View reviewed changes

Torch3333 mentioned this pull request Jun 8, 2023

Support conditions for transactional operations #837

Closed

brfrn169 reviewed Jun 9, 2023

View reviewed changes

.../java/com/scalar/db/transaction/consensuscommit/ConsensusCommitMutationOperationChecker.java Outdated Show resolved Hide resolved

Revise according to comments

afc2fbe

komamitsu reviewed Jun 13, 2023

View reviewed changes

core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransaction.java Outdated Show resolved Hide resolved

Torch3333 requested a review from brfrn169 June 13, 2023 03:02

komamitsu reviewed Jun 13, 2023

View reviewed changes

core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransaction.java Outdated Show resolved Hide resolved

komamitsu approved these changes Jun 13, 2023

View reviewed changes

Vincent Guilpain added 3 commits June 13, 2023 15:00

Use StringBuilder

494137e

Fix typo in a comment

562ca5f

Improve comments in code

8c34081

Torch3333 requested a review from komamitsu June 13, 2023 06:07

brfrn169 reviewed Jun 13, 2023

View reviewed changes

Merge branch 'master' of github.com:scalar-labs/scalardb into conditi…

7b2d59c

…on_for_transaction_2 # Conflicts: # docs/api-guide.md

komamitsu approved these changes Jun 13, 2023

View reviewed changes

feeblefakie reviewed Jun 14, 2023

View reviewed changes

josh-wong self-requested a review June 16, 2023 10:58

Throw UnsatisfiedConditionException when the mutation is executed

46441d2

Torch3333 force-pushed the condition_for_transaction_2 branch from 09b2ae1 to 46441d2 Compare June 19, 2023 06:10

Torch3333 requested review from feeblefakie and brfrn169 June 19, 2023 07:30

brfrn169 approved these changes Jun 20, 2023

View reviewed changes

josh-wong requested changes Jun 20, 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 Outdated Show resolved Hide resolved

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

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

Vincent Guilpain and others added 2 commits June 20, 2023 20:02

Merge branch 'master' of github.com:scalar-labs/scalardb into conditi…

a78ec29

…on_for_transaction_2 # Conflicts: # core/src/test/java/com/scalar/db/transaction/consensuscommit/CrudHandlerTest.java

Apply suggestions from Josh

a15aeb9

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

Torch3333 requested a review from josh-wong June 20, 2023 11:10

josh-wong approved these changes Jun 20, 2023

View reviewed changes

feeblefakie requested review from brfrn169 and josh-wong June 21, 2023 09:25

feeblefakie approved these changes Jun 21, 2023

View reviewed changes

josh-wong approved these changes Jun 22, 2023

View reviewed changes

Vincent Guilpain added 2 commits June 22, 2023 20:44

Move getFromReadSet method definition

c612740

Merge branch 'condition_for_transaction_2' of github.com:scalar-labs/…

1e27d78

…scalardb into condition_for_transaction_2

brfrn169 approved these changes Jun 23, 2023

View reviewed changes

Merge branch 'master' into condition_for_transaction_2

d38cc0e

brfrn169 merged commit 8bd43b9 into master Jun 23, 2023
12 checks passed

brfrn169 deleted the condition_for_transaction_2 branch June 23, 2023 04:44

brfrn169 pushed a commit that referenced this pull request Jul 7, 2023

Support condition for transactional operations (#899)

5efd392

Torch3333 mentioned this pull request Sep 29, 2023

Revise configuration-related docs (3.8) #1117

Merged

6 tasks

	// Build a condition
	MutationCondition condition =
	ConditionBuilder.putIf(ConditionBuilder.column("c4").isEqualToFloat(0.0F))
	.and(ConditionBuilder.column("c5").isEqualToDouble(0.0))
	.build();

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

	// Execute the Put operation
	transaction.put(put);

	// null, we consider the condition to be unsatisfied. This mimic the behavior as if
	// null, we consider the condition to be unsatisfied. This mimics the behavior as if

	* This validates the condition for the Put operation is satisfied.
	* This checks if the condition of the specified Put operation is satisfied for the specified record.

	public class ConditionalMutationValidator {
	public class MutationConditionsValidator {

Support condition for transactional operations #899

Support condition for transactional operations #899

Conversation

Torch3333 commented Jun 7, 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

Torch3333 Jun 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komamitsu commented Jun 13, 2023

komamitsu left a comment

Choose a reason for hiding this comment

Torch3333 commented Jun 13, 2023

brfrn169 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

feeblefakie Jun 15, 2023 • edited

Choose a reason for hiding this comment

brfrn169 Jun 15, 2023 • edited

Choose a reason for hiding this comment

Torch3333 Jun 16, 2023 • edited

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

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

Torch3333 Jun 19, 2023 • edited

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

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

Torch3333 commented Jun 19, 2023

brfrn169 left a comment

Choose a reason for hiding this comment

josh-wong left a comment

Choose a reason for hiding this comment

Torch3333 commented Jun 20, 2023

josh-wong left a 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

josh-wong left a comment

Choose a reason for hiding this comment

Torch3333 commented Jun 7, 2023 •

edited

Torch3333 Jun 13, 2023 •

edited

feeblefakie Jun 15, 2023 •

edited

brfrn169 Jun 15, 2023 •

edited

Torch3333 Jun 16, 2023 •

edited

Torch3333 Jun 19, 2023 •

edited