Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support set local region for RegionAwareEnsemblePlacementPolicy #3248

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Support set local region for RegionAwareEnsemblePlacementPolicy #3248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6dff264
support set local region for RegionAwareEnsemblePlacementPolicy
Apr 26, 2022
d4fd1ce
fix test code
Apr 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion ...server/src/main/java/org/apache/bookkeeper/client/RegionAwareEnsemblePlacementPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ public class RegionAwareEnsemblePlacementPolicy extends RackawareEnsemblePlaceme
public static final String REPP_DISABLE_DURABILITY_ENFORCEMENT_FEATURE = "reppDisableDurabilityEnforcementFeature";
public static final String REPP_ENABLE_VALIDATION = "reppEnableValidation";
public static final String REGION_AWARE_ANOMALOUS_ENSEMBLE = "region_aware_anomalous_ensemble";
public static final String REPP_LOCAL_REGION = "reppLocalRegion";
static final int MINIMUM_REGIONS_FOR_DURABILITY_DEFAULT = 2;
static final int REGIONID_DISTANCE_FROM_LEAVES = 2;
static final String UNKNOWN_REGION = "UnknownRegion";
Expand All @@ -79,7 +80,7 @@ public class RegionAwareEnsemblePlacementPolicy extends RackawareEnsemblePlaceme
protected Feature disableDurabilityFeature;
private int lastRegionIndex = 0;

RegionAwareEnsemblePlacementPolicy() {
protected RegionAwareEnsemblePlacementPolicy() {
Copy link
Member

Choose a reason for hiding this comment

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

why not change it to public?

Copy link
Author

Choose a reason for hiding this comment

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

Change to protected so that we could extends RegionAwareEnsemblePlacementPolicy and implement some addtion policy in broker side in future.
IMO no need change to public.

super();
perRegionPlacement = new HashMap<String, TopologyAwareEnsemblePlacementPolicy>();
address2Region = new ConcurrentHashMap<BookieId, String>();
Expand Down Expand Up @@ -173,6 +174,10 @@ public RegionAwareEnsemblePlacementPolicy initialize(ClientConfiguration conf,
super.initialize(conf, optionalDnsResolver, timer, featureProvider, statsLogger, bookieAddressResolver)
.withDefaultRack(NetworkTopology.DEFAULT_REGION_AND_RACK);
myRegion = getLocalRegion(localNode);
String localRegion = conf.getString(REPP_LOCAL_REGION, null);
Copy link
Member

Choose a reason for hiding this comment

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

simple code
String localRegion = conf.getString(REPP_LOCAL_REGION);
instead of
String localRegion = conf.getString(REPP_LOCAL_REGION, null);

image

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure whether throwExceptionOnMissing could be set true in some condition.

if (null != localRegion) {
myRegion = localRegion;
}
enableValidation = conf.getBoolean(REPP_ENABLE_VALIDATION, true);
// We have to statically provide regions we want the writes to go through and how many regions
// are required for durability. This decision cannot be driven by the active bookies as the
Expand Down
17 changes: 16 additions & 1 deletion ...er/src/test/java/org/apache/bookkeeper/client/TestRegionAwareEnsemblePlacementPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1208,16 +1208,31 @@ private void prepareNetworkTopologyForReorderTests(String myRegion) throws Excep

@Test
public void testBasicReorderReadSequenceWithLocalRegion() throws Exception {
prepareNetworkTopologyForReorderTests("region2");
basicReorderReadSequenceWithLocalRegionTest("region2", false);
}

@Test
public void testBasicReorderReadLACSequenceWithLocalRegion() throws Exception {
prepareNetworkTopologyForReorderTests("region2");
Copy link
Contributor

Choose a reason for hiding this comment

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

We should both test read and write.

Copy link
Author

@wangjialing218 wangjialing218 Jul 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

For your motivation, If we read all the entries from the configured region, it may cause hot bookie, and increase the quarantine rate of the hot bookie.

As current policy, read sequence is as follows

bookkeeper/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java

Line 810 in 4debbbf

* 1. available (local) bookies

This motivation just add a configuration to set the local region as myRegion for client, bookies in the same region will be orderd as 1. available (local) bookies, when these bookies become unavailable or slow, they will be orderd as 3. R* remaining (local) bookies or 6. slow bookies.

We should both test read and write.

This motivation just add a configuration related to the read order of bookies in a writeSet. IMO we do not need test write.

basicReorderReadSequenceWithLocalRegionTest("region2", true);
}

@Test
public void testBasicReorderReadSequenceWithLocalRegionConfig() throws Exception {
conf.setProperty("reppLocalRegion", "region2");
prepareNetworkTopologyForReorderTests("dummyRegion");
basicReorderReadSequenceWithLocalRegionTest("region2", false);
}

@Test
public void testBasicReorderReadLACSequenceWithLocalRegionConfig() throws Exception {
conf.setProperty("reppLocalRegion", "region2");
prepareNetworkTopologyForReorderTests("dummyRegion");
basicReorderReadSequenceWithLocalRegionTest("region2", true);
}

private void basicReorderReadSequenceWithLocalRegionTest(String myRegion, boolean isReadLAC) throws Exception {
prepareNetworkTopologyForReorderTests(myRegion);
List<BookieId> ensemble = repp.newEnsemble(9, 9, 5, null,
new HashSet<BookieId>()).getResult();
assertEquals(9, getNumCoveredRegionsInWriteQuorum(ensemble, 9));
Expand Down