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-20052 Release all locks locally on self primary replica expira… #2620

Merged
merged 6 commits into from
Sep 28, 2023

Conversation

vldpyatkov
Copy link
Contributor

@vldpyatkov vldpyatkov force-pushed the ignite-20052 branch 2 times, most recently from 1c96e09 to 92fab0d Compare September 25, 2023 15:14
@@ -51,4 +51,12 @@ public interface PlacementDriver extends EventProducer<PrimaryReplicaEvent, Prim
* @return Primary replica future.
*/
CompletableFuture<ReplicaMeta> getPrimaryReplica(ReplicationGroupId replicationGroupId, HybridTimestamp timestamp);

/**
* Returns a future that completes when all expiration listener of previous primary complete.
Copy link
Contributor

Choose a reason for hiding this comment

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

listeners

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, rewrited the javadoc.

));

if (prev != null && !prev.isDone()) {
LOG.warn("Previous lease expiration process has not completed yet [grpId={}]", grpId);
Copy link
Contributor

Choose a reason for hiding this comment

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

is it okay that we ignore the previous notification? What are the cases when it's possible?
Cannot we have smth like this?
imagine we have a few listeners, A, B and C
and the execution is:
notify(A, old)
notify(A, new)
notify(B, old)
notify(B, new)
notify(C, new)
notify(C, old)

Copy link
Contributor Author

@vldpyatkov vldpyatkov Sep 27, 2023

Choose a reason for hiding this comment

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

It could happens in the case when a lease expired, but the lease does not accepeted. I think again about it and desided to no trigger the expire event in the case.
Currently, we can change this warning message to just checkoing assertion.


ArrayList<CompletableFuture<?>> futs = new ArrayList<>();

for (UUID txId : txCleanupReadyFutures.keySet()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Iteration over 'txCleanupReadyFutures.keySet()' may be replaced with 'entrySet()' iteration.

But strictly speaking, the processing of txCleanupReadyFutures is not atomic here.
If we allow to execute cleanup on non-primary replicas and cleanup this map (as stated in IGNITE-18617), this code might start to crash -
the keyset iterator will return a txId, but txCleanupReadyFutures.get(txId) might return null

Copy link
Contributor Author

@vldpyatkov vldpyatkov Sep 27, 2023

Choose a reason for hiding this comment

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

Rewrote the place to the Map#compute invocation. I hope it allows us to not think about CME.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you.
What do you think if we change the code a bit?

 txCleanupReadyFutures.compute(txId, (id, txOps) -> {
                    if (txOps == null || isFinalState(txOps.state)) {
                        return null;
                    }

                    if (!txOps.futures.isEmpty()) {
                        CompletableFuture<?>[] txFuts = txOps.futures.values().stream()
                                .flatMap(Collection::stream)
                                .toArray(CompletableFuture[]::new);

                        futs.add(allOf(txFuts).whenComplete((unused, throwable) -> releaseTxLocks(txId)));
                    }

                    return txOps;
                });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

@vldpyatkov vldpyatkov merged commit 4a0e961 into apache:main Sep 28, 2023
1 check passed
@vldpyatkov vldpyatkov deleted the ignite-20052 branch September 28, 2023 09:09
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