Fix bugs in DefaultEnsemblePlacementPolicy #1788
Conversation
- bookieInfoMap is not initialized and newEnsemble will throws BKNotEnoughBookiesException if diskWeightBasedPlacement is enabled - add test coverage for DefaultEnsemblePlacementPolicy with diskWeightBasedPlacement enabled
| @@ -92,6 +94,9 @@ | |||
| } | |||
| newBookies.add(b); | |||
| --ensembleSize; | |||
| if (ensembleSize == 0) { | |||
| return newBookies; | |||
There was a problem hiding this comment.
simple return newBookies; after the while loop should be enough.
There was a problem hiding this comment.
may be, but i just want to follow the existing code format here, like how it is dealt in else block
| @@ -92,6 +94,9 @@ | |||
| } | |||
There was a problem hiding this comment.
is it possible for weightedSelection.getNextRandom() to return exactly the same one bookie which i.e. is already in the newBookies or excludeBookies thus resulting in the infinite loop?
There was a problem hiding this comment.
getNextRandom should eventually return different bookie.
check the logic here - https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/WeightedRandomSelection.java#L149
There was a problem hiding this comment.
what if all the bookies already in either in excludeBookies or in a newBookies?
There was a problem hiding this comment.
there is check for this condition - https://github.com/apache/bookkeeper/pull/1788/files#diff-b8389e8cd72c81923a6c9aa82e119310L86
|
run bookkeeper-server replication tests |
Descriptions of the changes in this PR: - bookieInfoMap is not initialized and newEnsemble will throws BKNotEnoughBookiesException if diskWeightBasedPlacement is enabled - add test coverage for DefaultEnsemblePlacementPolicy with diskWeightBasedPlacement enabled Reviewers: Sijie Guo <sijie@apache.org>, Andrey Yegorov <None> This closes #1788 from reddycharan/defaultplacementfix (cherry picked from commit 398aa4c) Signed-off-by: Sijie Guo <sijie@apache.org>
Descriptions of the changes in this PR: - bookieInfoMap is not initialized and newEnsemble will throws BKNotEnoughBookiesException if diskWeightBasedPlacement is enabled - add test coverage for DefaultEnsemblePlacementPolicy with diskWeightBasedPlacement enabled Reviewers: Sijie Guo <sijie@apache.org>, Andrey Yegorov <None> This closes #1788 from reddycharan/defaultplacementfix (cherry picked from commit 398aa4c) Signed-off-by: Sijie Guo <sijie@apache.org>
Descriptions of the changes in this PR:
diskWeightBasedPlacement is enabled