You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "tmzk1005 (via GitHub)" <gi...@apache.org> on 2024/03/20 13:18:02 UTC
[PR] Issue 4200: fix flaky test DeferredSyncTest.testForceWillAdvanceLacOnlyUpToLastAcknoledgedWrite [bookkeeper]
tmzk1005 opened a new pull request, #4234:
URL: https://github.com/apache/bookkeeper/pull/4234
### Motivation
`DeferredSyncTest.testForceWillAdvanceLacOnlyUpToLastAcknoledgedWrite` is flaky. The build times out since the test MAY never finishes. This PR fix it.
### Changes
Use `synchronized` block to operate `suspendedBookiesForForceLedgerAcks` and `deferredBookieForceLedgerResponses`
Master Issue: #4220
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 4200: fix flaky test DeferredSyncTest.testForceWillAdvanceLacOnlyUpToLastAcknoledgedWrite [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #4234:
URL: https://github.com/apache/bookkeeper/pull/4234#discussion_r1532204972
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockBookKeeperTestCase.java:
##########
@@ -303,8 +303,12 @@ protected void suspendBookieForceLedgerAcks(BookieId address) {
}
protected void resumeBookieWriteAcks(BookieId address) {
- suspendedBookiesForForceLedgerAcks.remove(address);
- List<Runnable> pendingResponses = deferredBookieForceLedgerResponses.remove(address);
+ List<Runnable> pendingResponses;
+ synchronized (address) {
Review Comment:
I guess it makes sense since it would prevent the possible data race later, in the other location where changes are made. Perhaps adding a code comment would be helpful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 4200: fix flaky test DeferredSyncTest.testForceWillAdvanceLacOnlyUpToLastAcknoledgedWrite [bookkeeper]
Posted by "tmzk1005 (via GitHub)" <gi...@apache.org>.
tmzk1005 commented on code in PR #4234:
URL: https://github.com/apache/bookkeeper/pull/4234#discussion_r1532238863
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockBookKeeperTestCase.java:
##########
@@ -303,8 +303,12 @@ protected void suspendBookieForceLedgerAcks(BookieId address) {
}
protected void resumeBookieWriteAcks(BookieId address) {
- suspendedBookiesForForceLedgerAcks.remove(address);
- List<Runnable> pendingResponses = deferredBookieForceLedgerResponses.remove(address);
+ List<Runnable> pendingResponses;
+ synchronized (address) {
Review Comment:
Use field `suspendedBookiesForForceLedgerAcks` or just `this` as the object monitor would be ok too, but data race happens on the same `BookieId` instance causes the problem, so I used it as the object monitor.
I will add a comment.
Thanks~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 4200: fix flaky test DeferredSyncTest.testForceWillAdvanceLacOnlyUpToLastAcknoledgedWrite [bookkeeper]
Posted by "nicoloboschi (via GitHub)" <gi...@apache.org>.
nicoloboschi merged PR #4234:
URL: https://github.com/apache/bookkeeper/pull/4234
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 4200: fix flaky test DeferredSyncTest.testForceWillAdvanceLacOnlyUpToLastAcknoledgedWrite [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #4234:
URL: https://github.com/apache/bookkeeper/pull/4234#discussion_r1532202712
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockBookKeeperTestCase.java:
##########
@@ -303,8 +303,12 @@ protected void suspendBookieForceLedgerAcks(BookieId address) {
}
protected void resumeBookieWriteAcks(BookieId address) {
- suspendedBookiesForForceLedgerAcks.remove(address);
- List<Runnable> pendingResponses = deferredBookieForceLedgerResponses.remove(address);
+ List<Runnable> pendingResponses;
+ synchronized (address) {
Review Comment:
just wondering why the `BookieId` instance is used as the object monitor. Do you have a specific explanation?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 4200: fix flaky test DeferredSyncTest.testForceWillAdvanceLacOnlyUpToLastAcknoledgedWrite [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #4234:
URL: https://github.com/apache/bookkeeper/pull/4234#issuecomment-2012815845
@nicoloboschi @eolivelli Please review and merge since this fixes a problem that is very common in CI.
For example, it failed in this build very recently: https://github.com/apache/bookkeeper/actions/runs/8375785440/job/22934347745#step:11:419
The build will hang for 75 minutes when this happens. A timeout would also be great.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org