You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2020/05/17 14:42:54 UTC
[GitHub] [bookkeeper] eolivelli opened a new pull request #2333: QuorumCoverage should only count unknown nodes
eolivelli opened a new pull request #2333:
URL: https://github.com/apache/bookkeeper/pull/2333
The original patch was contributed by @ivankelly in PR #2303, I have only fixed checkstyle and removed two tests that were wrong.
Quorum coverage checks if we have heard from enough nodes to know that
there is no entry that can have been written to enough nodes that we
haven't heard from to have formed an ack quorum.
The coverage algorithm was correct pre-5e399df.
5e399df(BOOKKEEPER-759: Delay Ensemble Change & Disable Ensemble
Change) broke this, but it still seems to have worked because they had
a broken else statement at the end. Why a change which is 100% about
the write-path changed something in the read-path is a mystery.
dcdd1e(Small fix wrong nodesUninitialized count when checkCovered)
went on to fix the broken fix, so the whole thing ended up broke.
The change also modifies ReadLastConfirmedOp to make it testable.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lamber-ken commented on pull request #2333: QuorumCoverage should only count unknown nodes
Posted by GitBox <gi...@apache.org>.
lamber-ken commented on pull request #2333:
URL: https://github.com/apache/bookkeeper/pull/2333#issuecomment-631512593
> That test was wrong.
> It used ensemble size = 4 and write quorum size = 1.
> Then it shutdown a bookie.
> If it shuts down the only bookie that contains the entry the test will fail
right, 💯
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2333: QuorumCoverage should only count unknown nodes
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2333:
URL: https://github.com/apache/bookkeeper/pull/2333#issuecomment-629809563
@sijie @rdhabalia @merlimat you already approved the original patch
Please any of you take a look and confirm your approval
my addition is
https://github.com/apache/bookkeeper/pull/2333/commits/55607a874738b02457d309291b87713a077b3e33
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2333: QuorumCoverage should only count unknown nodes
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2333:
URL: https://github.com/apache/bookkeeper/pull/2333#issuecomment-630607876
@sijie did all CI tests pass this time ?
last time I saw that testZoneawarePlacementPolicyCheck did not pass.
I wouldn't want that we have introduced a new annoying flaky test
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2333: QuorumCoverage should only count unknown nodes
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2333:
URL: https://github.com/apache/bookkeeper/pull/2333#issuecomment-631507628
That test was wrong.
It used ensemble size = 4 and write quorum size = 1.
Then it shutdown a bookie.
If it shuts down the only bookie that contains the entry the test will fail
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] sijie commented on pull request #2333: QuorumCoverage should only count unknown nodes
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #2333:
URL: https://github.com/apache/bookkeeper/pull/2333#issuecomment-630650563
@eolivelli all CI tests passed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2333: QuorumCoverage should only count unknown nodes
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2333:
URL: https://github.com/apache/bookkeeper/pull/2333#issuecomment-629998201
It looks like I still have a test to fix:
```
[ERROR] testZoneawarePlacementPolicyCheck(org.apache.bookkeeper.replication.AuditorPlacementPolicyCheckTest) Time elapsed: 61.135 s <<< FAILURE!
java.lang.AssertionError: placementPolicyCheck should have executed
at org.apache.bookkeeper.replication.AuditorPlacementPolicyCheckTest.startAuditorAndWaitForPlacementPolicyCheck(AuditorPlacementPolicyCheckTest.java:684)
at org.apache.bookkeeper.replication.AuditorPlacementPolicyCheckTest.testZoneawarePlacementPolicyCheck(AuditorPlacementPolicyCheckTest.java:628)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lamber-ken commented on pull request #2333: QuorumCoverage should only count unknown nodes
Posted by GitBox <gi...@apache.org>.
lamber-ken commented on pull request #2333:
URL: https://github.com/apache/bookkeeper/pull/2333#issuecomment-631377461
hi @eolivelli, I have a question that why remove following code snippet? 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] rdhabalia commented on pull request #2333: QuorumCoverage should only count unknown nodes
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #2333:
URL: https://github.com/apache/bookkeeper/pull/2333#issuecomment-763909333
just fyi: fix: https://github.com/apache/bookkeeper/pull/2333
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lamber-ken edited a comment on pull request #2333: QuorumCoverage should only count unknown nodes
Posted by GitBox <gi...@apache.org>.
lamber-ken edited a comment on pull request #2333:
URL: https://github.com/apache/bookkeeper/pull/2333#issuecomment-631377461
hi @eolivelli, I have a question that why remove following code snippet? thanks
After i learning the mechanism of bookkeeper, i tried to fix this flaky test case.
please go ahead with https://github.com/apache/bookkeeper/pull/2337 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] rdhabalia commented on pull request #2333: QuorumCoverage should only count unknown nodes
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #2333:
URL: https://github.com/apache/bookkeeper/pull/2333#issuecomment-763909333
just fyi: fix: https://github.com/apache/bookkeeper/pull/2333
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] sijie merged pull request #2333: QuorumCoverage should only count unknown nodes
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #2333:
URL: https://github.com/apache/bookkeeper/pull/2333
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org