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-21371 Add tx coordinator id to TX requests #3128

Merged
merged 9 commits into from
Feb 5, 2024

Conversation

Cyrill
Copy link
Contributor

@Cyrill Cyrill commented Jan 31, 2024

*
* @return the transaction coordinator inconsistent ID
*/
String txCoordinatorId();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need txCoordinatorId in TxFinishReplicaRequest? Use-cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See PartitionListener.handleFinishTxCommand, line 351:

markFinished(txId, cmd.commit(), cmd.commitTimestamp(), cmd.txCoordinatorId());

Copy link
Contributor

@sanpwc sanpwc Feb 2, 2024

Choose a reason for hiding this comment

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

I mean logically. Why do we need coordinatorId on finish request? From my point of view, we use coordinatorId retrieved from txnState local map in two cases only.

  1. In order to check whether coordinator is dead and trigger the recovey, which definitely has no sense in case of finished transaction.
  2. In order to decrement counter of an "in-flight" transactions that is used within index build procedure. In that case it's guaranteed that txnState local map will be populated with txCoordId prior to finish processing.

All in all, I believe that we may remove txCoordinatorId() from TxFinishReplicaRequest and update

    private void markFinished(UUID txId, boolean commit, @Nullable HybridTimestamp commitTimestamp) {
        txManager.updateTxMeta(txId, old -> new TxStateMeta(
                commit ? COMMITTED : ABORTED,
                old == null ? null : old.txCoordinatorId(),
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same applies to WriteIntentSwitch command handling

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.

LGTM

@sanpwc sanpwc merged commit 8880aa1 into apache:main Feb 5, 2024
1 check passed
@Cyrill Cyrill deleted the IGNITE-21371 branch February 5, 2024 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants