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-17834 Fixed. #1168

Merged
merged 7 commits into from
Oct 7, 2022
Merged

Ignite-17834 Fixed. #1168

merged 7 commits into from
Oct 7, 2022

Conversation

sanpwc
Copy link
Contributor

@sanpwc sanpwc commented Oct 6, 2022

Signed-off-by: alapin lapin1702@gmail.com

sanpwc added 2 commits October 6, 2022 11:36
Signed-off-by: alapin <lapin1702@gmail.com>
Signed-off-by: alapin <lapin1702@gmail.com>
} catch (TransactionException enlistResultException) {
return tx0.rollbackAsync().handle((ignored, err) -> {
if (err != null) {
e.addSuppressed(err);
Copy link
Contributor

Choose a reason for hiding this comment

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

e is always null here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

try {
tx0.enlistResultFuture(fut);
} catch (TransactionException enlistResultException) {
return tx0.rollbackAsync().handle((ignored, err) -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you rolling back the transaction if it is already finished?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it's required to cleanup locks and writeIntents for operation that failed to enlistResultFuture.

Copy link
Contributor

Choose a reason for hiding this comment

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

So that will not happen because the transaction is already finished.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right we need an escape path in order to rollback partially finished transaction.

private final List<CompletableFuture<?>> enlistedResults = new ArrayList<>();

/** Guard that prevents finishing or enlisting new operations into already finished or finishing transaction. */
private boolean txInFinishState = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Way do not you want to make txInFinishState to AtomicBoolean and get rid of synchronized in the finish method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it's required not only to change txInFinishState atomically but also to populate enlistedResults with new value

    public synchronized void enlistResultFuture(CompletableFuture<?> resultFuture) {
        if (txInFinishState) {
            throw new TransactionException(TX_ALREADY_FINISHED_ERR,
                    "Failed to enlist operation into already finished transaction txId={" + id + '}');
        }

        enlistedResults.add(resultFuture);
    }

sanpwc added 5 commits October 6, 2022 21:20
Signed-off-by: alapin <lapin1702@gmail.com>
This reverts commit 357e3f9.

Signed-off-by: alapin <lapin1702@gmail.com>

# Conflicts:
#	modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java
This reverts commit 357e3f9.

Signed-off-by: alapin <lapin1702@gmail.com>

# Conflicts:
#	modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java
@sanpwc sanpwc merged commit 4c903a9 into apache:main Oct 7, 2022
@sanpwc sanpwc deleted the ignite-17834 branch October 7, 2022 09:41
lowka pushed a commit to gridgain/apache-ignite-3 that referenced this pull request Mar 18, 2023
Signed-off-by: alapin <lapin1702@gmail.com>
Co-authored-by: sanpwc <alapin@gridgain.com>
lowka pushed a commit to gridgain/apache-ignite-3 that referenced this pull request Apr 19, 2023
Signed-off-by: alapin <lapin1702@gmail.com>
Co-authored-by: sanpwc <alapin@gridgain.com>
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