-
Notifications
You must be signed in to change notification settings - Fork 902
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
Support set local region for RegionAwareEnsemblePlacementPolicy #3248
base: master
Are you sure you want to change the base?
Conversation
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.
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.
basicReorderReadSequenceWithLocalRegionTest("region2", false); | ||
} | ||
|
||
@Test | ||
public void testBasicReorderReadLACSequenceWithLocalRegion() throws Exception { | ||
prepareNetworkTopologyForReorderTests("region2"); |
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.
We should both test read and write.
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.
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
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.
rerun failure checks |
@@ -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); |
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.
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'm not sure whether throwExceptionOnMissing
could be set true in some condition.
@@ -79,7 +80,7 @@ public class RegionAwareEnsemblePlacementPolicy extends RackawareEnsemblePlaceme | |||
protected Feature disableDurabilityFeature; | |||
private int lastRegionIndex = 0; | |||
|
|||
RegionAwareEnsemblePlacementPolicy() { | |||
protected RegionAwareEnsemblePlacementPolicy() { |
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.
why not change it to public?
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.
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.
5c75626
to
d4fd1ce
Compare
fix old workflow,please see #3455 for detail |
What alternative solutions have you looked at?
AFACT, the dummy bookie node is created to make use of the resolver API that takes BookieNode as a parameter. |
In synchronous geo-replication solution, suppose we have 4 bookies in two regions, bk1 and bk2 with rack setting
It's okey to resolve the broker's address to region/rack to achieve, we need to configure the resolver and it's not an easy way if brokers deployed in kubernetes. |
@wangjialing218 It is easy to configure by If we introduce a configuration |
Motivation
When RegionAwareEnsemblePlacementPolicy is enabled, write set of one entry contains bookie nodes in different region.
When bookkeeper client read entries, the policy provide a read sequence to read from bookies in the same region first. see
bookkeeper/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
Line 834 in 4debbbf
It depend on
myRegion
which indicate the local region of bookkeeper client (mostly pulsar broker).Local region is not a configurable value, and it's generated by local host address. see
bookkeeper/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
Line 208 in 4debbbf
It's okey when there is a bookie in the same host of broker.
But in most case, brokers were deployed in other nodes (or pod in kubernetes), and bookkeeper client will fail to get local region value.
Changes
Add configuration of local region for bookkeeper client.