Skip to content

Fix MLTransactionMetadataStore.update async fail #14532

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.

Sign up for GitHub

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

Jump to bottom
Merged
hezhangjian merged 1 commit into from
Mar 3, 2022
Merged

Fix MLTransactionMetadataStore.update async fail #14532

hezhangjian merged 1 commit into from
Mar 3, 2022

Conversation

@shibd
Copy link
Member

@shibd shibd commented Mar 2, 2022

Motivation

#14525

When update states is TxnStatus.COMMITTED, Not correctly completableFuture.complete.

Modifications

  • When update states is TxnStatus.COMMITTED, Add return to ending. Avoid direct calls completableFuture.complete from other logic.

Documentation

  • [x ] no-need-doc

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 2, 2022
Jason918
Jason918 reviewed Mar 2, 2022
View reviewed changes
Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

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

LGTM

@shibd
Copy link
Member Author

shibd commented Mar 2, 2022

/pulsarbot run-failure-checks

eolivelli
eolivelli approved these changes Mar 2, 2022
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Good catch

@shibd
Copy link
Member Author

shibd commented Mar 3, 2022

/pulsarbot run-failure-checks

@shibd
Copy link
Member Author

shibd commented Mar 3, 2022

/pulsarbot run-failure-checks

@lhotari
Copy link
Member

lhotari commented Mar 3, 2022

@shibd Instead of re-running checks, you will need to close and reopen this PR so that the build picks up the fix #14524 . There was a thread leak which also caused a memory leak and made the tests run extremely slow because of reduced working memory (which leads to very frequent GCs and a lot of pauses).

@shibd
Copy link
Member Author

shibd commented Mar 3, 2022 •
edited
Loading

@shibd Instead of re-running checks, you will need to close and reopen this PR so that the build picks up the fix #14524 . There was a thread leak which also caused a memory leak and made the tests run extremely slow because of reduced working memory (which leads to very frequent GCs and a lot of pauses).

@lhotari Thank your reply, I see that CI has passed.

@hezhangjian hezhangjian merged commit c15d0ef into apache:master Mar 3, 2022
@shibd shibd deleted the feat-14525 branch March 3, 2022 08:37
@lhotari
Copy link
Member

lhotari commented Mar 3, 2022

I see that CI has passed.

great!

@codelipenghui codelipenghui added this to the 2.11.0 milestone Mar 4, 2022
@gaoran10 gaoran10 added release/2.9.2 cherry-picked/branch-2.9 Archived: 2.9 is end of life and removed release/2.9.3 labels Mar 11, 2022
gaoran10 pushed a commit that referenced this pull request Mar 11, 2022
@shibd @gaoran10
[Flaky-test]: Fix MLTransactionMetadataStoreTest.testInitTransactionR…
5a03b36 
…eader fails sporadically (#14532)

### Motivation

#14525

When update states is `TxnStatus.COMMITTED`,  Not correctly `completableFuture.complete`.

### Modifications
- When update states is `TxnStatus.COMMITTED`, Add return to ending. Avoid direct calls `completableFuture.complete` from other logic.

### Documentation
- [x ] `no-need-doc`

(cherry picked from commit c15d0ef)
codelipenghui pushed a commit that referenced this pull request Mar 12, 2022
@shibd @codelipenghui
[Flaky-test]: Fix MLTransactionMetadataStoreTest.testInitTransactionR…
cafeade 
…eader fails sporadically (#14532)

### Motivation

#14525

When update states is `TxnStatus.COMMITTED`,  Not correctly `completableFuture.complete`.

### Modifications
- When update states is `TxnStatus.COMMITTED`, Add return to ending. Avoid direct calls `completableFuture.complete` from other logic.

### Documentation
- [x ] `no-need-doc`

(cherry picked from commit c15d0ef)
@codelipenghui codelipenghui modified the milestones: 2.11.0, 2.10.0 Mar 12, 2022
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
@shibd
[Flaky-test]: Fix MLTransactionMetadataStoreTest.testInitTransactionR…
015190e 
…eader fails sporadically (apache#14532)

### Motivation

apache#14525 

When update states is `TxnStatus.COMMITTED`,  Not correctly `completableFuture.complete`.

### Modifications
- When update states is `TxnStatus.COMMITTED`, Add return to ending. Avoid direct calls `completableFuture.complete` from other logic.

### Documentation
- [x ] `no-need-doc`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Jason918 Jason918 Jason918 approved these changes

@eolivelli eolivelli eolivelli approved these changes

@hezhangjian hezhangjian hezhangjian approved these changes

@liudezhi2098 liudezhi2098 liudezhi2098 approved these changes

Assignees

@shibd shibd

Labels

area/transaction cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-not-needed Your PR changes do not impact docs release/2.9.2

Projects

None yet

Milestone

2.10.0

Development

Successfully merging this pull request may close these issues.

8 participants

@shibd @lhotari @Jason918 @eolivelli @hezhangjian @liudezhi2098 @codelipenghui @gaoran10