Skip to content

Fix flaky-test testWriteSetWriteableCheck#3555

Merged
hangc0276 merged 1 commit into
apache:masterfrom
wenbingshen:flaky-testWriteSetWriteableCheck
Oct 24, 2022
Merged

Fix flaky-test testWriteSetWriteableCheck#3555
hangc0276 merged 1 commit into
apache:masterfrom
wenbingshen:flaky-testWriteSetWriteableCheck

Conversation

@wenbingshen
Copy link
Copy Markdown
Member

Motivation

testWriteSetWriteableCheck always failed. After investigation, it is found that isWritable=false is set when the simulated channel is not writable, but the unit test cannot fully perceive the channel establishment, because the channel is completely established asynchronously. The isWritable state of false is changed to true again.

you can see the following logs:
image

The smallest change is that we can ensure that the channel is established before changing the channel state, such as executing bkc.getBookieInfo(), and then setTargetChannelState

@wenbingshen
Copy link
Copy Markdown
Member Author

ping @dlg99 @zymap @eolivelli @hangc0276 @StevenLuMT PTAL. Thanks.

@dlg99
Copy link
Copy Markdown
Contributor

dlg99 commented Oct 19, 2022

@wenbingshen Do you know what broke the test? I believe it was passing / not flaking on the CI.

@wenbingshen
Copy link
Copy Markdown
Member Author

I believe it was passing / not flaking on the CI.

@dlg99 Thanks for your comments. I've documented in my notes the failure of testWriteSetWriteableCheck in the following CI test, check it out:
https://github.com/apache/bookkeeper/actions/runs/3125949264/jobs/5070881119
https://github.com/apache/bookkeeper/actions/runs/3124965464/jobs/5068848112

As you can see it often fails. Likewise, when I tested my local, I ran 10 times and failed 7 times.
image

Do you know what broke the test?

Please check the picture below:

I printed the log before and after setting the channel isWritable flag . It can be seen that the test thread sets the channel to be unreadable, which occurs before the ChannelFuture listener makeWritable is triggered after the connection is established, causing the result of setTargetChannelState=false to be changed to true again.

image

Copy link
Copy Markdown
Contributor

@hangc0276 hangc0276 left a comment

Choose a reason for hiding this comment

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

Nice investigation!

@hangc0276 hangc0276 added this to the 4.16.0 milestone Oct 20, 2022
@hangc0276 hangc0276 merged commit c76e549 into apache:master Oct 24, 2022
@wenbingshen wenbingshen deleted the flaky-testWriteSetWriteableCheck branch October 24, 2022 02:50
zymap pushed a commit that referenced this pull request Oct 26, 2022
### Motivation
 `testWriteSetWriteableCheck` always failed. After investigation, it is found that `isWritable=false` is set when the simulated channel is not writable, but the unit test cannot fully perceive the channel establishment, because the channel is completely established asynchronously. The isWritable state of false is changed to true again.

you can see the following logs:
![image](https://user-images.githubusercontent.com/35599757/196625314-0ea5a305-9564-4738-8055-20baa6a5c5a2.png)

The smallest change is that we can ensure that the channel is established before changing the channel state, such as executing `bkc.getBookieInfo()`, and then `setTargetChannelState`

(cherry picked from commit c76e549)
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
### Motivation
 `testWriteSetWriteableCheck` always failed. After investigation, it is found that `isWritable=false` is set when the simulated channel is not writable, but the unit test cannot fully perceive the channel establishment, because the channel is completely established asynchronously. The isWritable state of false is changed to true again.

you can see the following logs:
![image](https://user-images.githubusercontent.com/35599757/196625314-0ea5a305-9564-4738-8055-20baa6a5c5a2.png)

The smallest change is that we can ensure that the channel is established before changing the channel state, such as executing `bkc.getBookieInfo()`, and then `setTargetChannelState`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants