-
Notifications
You must be signed in to change notification settings - Fork 950
Issue 1898: Implement isEnsembleAdheringToPlacementPolicy
in RegionAwareEnsemblePlacementPolicy
#4133
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.
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
base: master
Are you sure you want to change the base?
Conversation
@horizonzy Please help take a look at this PR, thanks. |
@dragonls Thanks for your contribution. Would you please fix the failed check style? thanks. |
Fixed. |
...er-server/src/main/java/org/apache/bookkeeper/client/RegionAwareEnsemblePlacementPolicy.java
Show resolved
Hide resolved
...er-server/src/main/java/org/apache/bookkeeper/client/RegionAwareEnsemblePlacementPolicy.java
Outdated
Show resolved
Hide resolved
return PlacementPolicyAdherence.FAIL; | ||
} | ||
|
||
if (regionsInQuorum.size() < 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question: why judge the regionsInQuorum.size() < 2
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of doing this here is to align the implement in org.apache.bookkeeper.client.RegionAwareEnsemblePlacementPolicy#newEnsemble
:
Lines 356 to 366 in 185fef6
// Single region, fall back to RackAwareEnsemblePlacement | |
if (numRegionsAvailable < 2) { | |
RRTopologyAwareCoverageEnsemble ensemble = new RRTopologyAwareCoverageEnsemble(ensembleSize, | |
writeQuorumSize, ackQuorumSize, REGIONID_DISTANCE_FROM_LEAVES, | |
effectiveMinRegionsForDurability > 0 ? new HashSet<>(perRegionPlacement.keySet()) : null, | |
effectiveMinRegionsForDurability, minNumRacksPerWriteQuorum); | |
TopologyAwareEnsemblePlacementPolicy nextPolicy = perRegionPlacement.get( | |
availableRegions.iterator().next()); | |
return nextPolicy.newEnsemble(ensembleSize, writeQuorumSize, writeQuorumSize, | |
comprehensiveExclusionBookiesSet, ensemble, ensemble); | |
} |
When there is only one available region, fall back to RackAwareEnsemblePlacement
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little weird.
Based on: writeQuorumSize = 3.
If regionsInQuorum < 2, the result may be MEETS_STRICT
.
If regionsInQuorum = 2, the result is FAIL
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, a little weird.
Should we just remove the numRegionsAvailable < 2
judge in isEnsembleAdheringToPlacementPolicy
? May I have your suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PlacementPolicyAdherence enum has three values FAIL(1), MEETS_SOFT(3), MEETS_STRICT(5)
.
If there are enough different region, the result could be MEETS_STRICT
. If there aren't enough different region, the each RackAwarePLacementPolicy MEETS_STRICT
, the result could be MEETS_SOFT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
comprehensiveExclusionBookiesSet, ensemble, ensemble); | ||
return PlacementResult.of(placementResult.getResult(), | ||
isEnsembleAdheringToPlacementPolicy( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, we can simplify the logic. We already know there is only one region.
Just need check the result of nextPolicy.newEnsemble
.
- If the result is PlacementPolicyAdherence.FAIL, return directly.
- If the result is PlacementPolicyAdherence.MEETS_STRICT, then check whether minNumRacksPerWriteQuorum is more than 1. If it is, return PlacementPolicyAdherence.SOFT, otherwise return PlacementPolicyAdherence.MEETS_STRICT.
Master Issue: #1898
Motivation
Before this PR,
isEnsembleAdheringToPlacementPolicy
inRegionAwareEnsemblePlacementPolicy
always returnPlacementPolicyAdherence.MEETS_STRICT
, which is not good to keep data highly available while usingRegionAwareEnsemblePlacementPolicy
, especially when one region is down.Changes
Implement
isEnsembleAdheringToPlacementPolicy
inRegionAwareEnsemblePlacementPolicy
.The main implementation idea is that when all allocations are satisfied in different regions, it is considered
PlacementPolicyAdherence.MEETS_STRICT
, otherwisePlacementPolicyAdherence.FAIL
.