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.

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

Make ZkBookieRackAffinityMapping work as expected #6917

Merged
merged 3 commits into from
May 19, 2020

Conversation

sijie
Copy link
Member

@sijie sijie commented May 8, 2020

Motivation

The current bookie rack affinity logic is problematic. The rack map is expecting a host:port pair but the bookkeeper DNS resolver is expecting a hostname or a host address.

Modification

  • Introduce a new HashMap to keep a mapping between hostname and bookie info. It maintains the mapping for bookkeeper DNS resolver to lookup the network location.

Verification

I have verified it worked well in setting up rackware placement policy.

@sijie sijie added this to the 2.6.0 milestone May 8, 2020
@sijie sijie self-assigned this May 8, 2020
@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

@sijie
Copy link
Member Author

sijie commented May 8, 2020

/pulsarbot run-failure-checks

3 similar comments
@jiazhai
Copy link
Member

jiazhai commented May 9, 2020

/pulsarbot run-failure-checks

@jiazhai
Copy link
Member

jiazhai commented May 11, 2020

/pulsarbot run-failure-checks

@jiazhai
Copy link
Member

jiazhai commented May 11, 2020

/pulsarbot run-failure-checks

@jiazhai
Copy link
Member

jiazhai commented May 11, 2020

@sijie Seems the test failure is related with this change:
https://github.com/apache/pulsar/pull/6917/checks?check_run_id=662411026

[ERROR] Tests run: 6, Failures: 3, Errors: 0, Skipped: 3, Time elapsed: 5.004 s <<< FAILURE! - in org.apache.pulsar.zookeeper.ZkBookieRackAffinityMappingTest
[ERROR] testBasic(org.apache.pulsar.zookeeper.ZkBookieRackAffinityMappingTest)  Time elapsed: 0.028 s  <<< FAILURE!
java.lang.RuntimeException: java.net.UnknownHostException: 127.0.0.1
	at org.apache.pulsar.zookeeper.ZkBookieRackAffinityMapping.lambda$1(ZkBookieRackAffinityMapping.java:103)
	at java.util.TreeMap.forEach(TreeMap.java:1005)
	at org.apache.pulsar.zookeeper.ZkBookieRackAffinityMapping.lambda$0(ZkBookieRackAffinityMapping.java:85)
	at java.util.TreeMap.forEach(TreeMap.java:1005)
	at org.apache.pulsar.zookeeper.ZkBookieRackAffinityMapping.updateRacksWithHost(ZkBookieRackAffinityMapping.java:84)
	at org.apache.pulsar.zookeeper.ZkBookieRackAffinityMapping.getRack(ZkBookieRackAffinityMapping.java:189)
	at org.apache.pulsar.zookeeper.ZkBookieRackAffinityMapping.resolve(ZkBookieRackAffinityMapping.java:165)
	at org.apache.pulsar.zookeeper.ZkBookieRackAffinityMappingTest.testBasic(ZkBookieRackAffinityMappingTest.java:83)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:124)
	at org.testng.internal.InvokeMethodRunnable.runOne(InvokeMethodRunnable.java:54)
	at org.testng.internal.InvokeMethodRunnable.run(InvokeMethodRunnable.java:44)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.net.UnknownHostException: 127.0.0.1
	at org.apache.bookkeeper.net.BookieSocketAddress.<init>(BookieSocketAddress.java:68)

if (biOpt.isPresent()) {
bi = biOpt.get();
} else {
updateRacksWithHost(racksWithHost);
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to update it here? seems this is a get operation, If it is not present, there is no need to update?

@jiazhai
Copy link
Member

jiazhai commented May 13, 2020

would like to remove it out of 2.5.2

@sijie sijie added component/storage type/bug The PR fixed a bug or issue reported a bug labels May 18, 2020
@sijie
Copy link
Member Author

sijie commented May 18, 2020

@jiazhai: fix the comment. the BookiesRackConfiguration expects the key to be hostname:port. We used bookieInfoMap for resolving the rack information.

@sijie sijie merged commit 5d0e3b3 into apache:master May 19, 2020
sijie added a commit to streamnative/pulsar-archived that referenced this pull request May 26, 2020
### Motivation

The current bookie rack affinity logic is problematic. The rack map is expecting a `host:port` pair but the bookkeeper DNS resolver is expecting a hostname or a host address. 

### Modification

- Introduce a new HashMap to keep a mapping between hostname and bookie info. It maintains the mapping for bookkeeper DNS resolver to lookup the network location.
Huanli-Meng pushed a commit to Huanli-Meng/pulsar that referenced this pull request May 27, 2020
### Motivation

The current bookie rack affinity logic is problematic. The rack map is expecting a `host:port` pair but the bookkeeper DNS resolver is expecting a hostname or a host address. 

### Modification

- Introduce a new HashMap to keep a mapping between hostname and bookie info. It maintains the mapping for bookkeeper DNS resolver to lookup the network location.
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
### Motivation

The current bookie rack affinity logic is problematic. The rack map is expecting a `host:port` pair but the bookkeeper DNS resolver is expecting a hostname or a host address. 

### Modification

- Introduce a new HashMap to keep a mapping between hostname and bookie info. It maintains the mapping for bookkeeper DNS resolver to lookup the network location.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants