Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IGNITE-23748 Lock LWM when executing RO operation on data node #4974

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

rpuch
Copy link
Contributor

@rpuch rpuch commented Dec 26, 2024

https://issues.apache.org/jira/browse/IGNITE-23748

  • When executing an operation in an RO transaction (explicit or implicit), attempt to lock LWM on the data node where it's executed
  • Direct RO operations (which happen in implicit transactions) only lock LWM if they concern more than 1 key
  • If lock attempt fails, throw an exception with a specific error code
  • When cleaning up after an RO transaction had been finished, unlock LWM on each node where such a cleanup happens
  • For direct RO operations, unlock LWM right after the read has been done on the data node
  • When locking LWM for an explicit transaction, also register a hook to unlock LWM when the transaction coordinator leaves

@rpuch rpuch force-pushed the ignite-23748 branch 3 times, most recently from 056d370 to 9ff9923 Compare December 26, 2024 19:23
* When executing an operation in an RO transaction (explicit or implicit), attempt to lock LWM on the data node where it's executed
* If lock attempt fails, throw an exception with a specific error code
* When cleaning up after an RO transaction had been finished, unlock LWM on each node where such a cleanup happens
* For direct RO operations (which happen in implicit RO transactions), unlock LWM right after the read has been done on the data node
Copy link
Contributor

@sanpwc sanpwc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have benchmark results that will show the difference for RO performance with and without proposed logic?

@@ -534,7 +540,7 @@ private CompletableFuture<?> processRequest(ReplicaRequest request, @Nullable Bo
// Don't need to validate schema.
if (opTs == null) {
assert opTsIfDirectRo == null;
return processOperationRequestWithTxRwCounter(senderId, request, isPrimary, null, leaseStartTime);
return processOperationRequestWithWrappingLogic(senderId, request, isPrimary, null, leaseStartTime);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the postfix "WithWrappingLogic", wrapping is very general term and thus such postfix says nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method now adds wrapping related to:

  1. RW operation counting
  2. RO operation LWM locking/unlocking

The old name only mentioned 1 and now became 'not completely true'. Adding another suffix to mention RO+LWM to the existing one would produce a monstrosity. That's why I chose a generic name that says that it adds some wrapping.

Do you have a specific idea on how to improve this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a specific idea on how to improve this?

Nothing good, e.g. processOperationRequestWithTxResourceManagementWrappingLogic. Up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to processOperationRequestWithTxOperationManagementLogic() (one word shorter than you suggested as it was too ugly wrt line breaks)

@Nullable
UUID transactionId();

/** Inconsistent ID of transaction to which this operation belongs. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent ID of transaction coordinator I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants