Validate rack name #14336

hangc0276 · 2022-02-17T05:42:07Z

Motivation

If the rack name set in the following case:

Enabled rack aware placement policy, and user set rack name which contains "/" in addition to the head and tail of the string. Such as "/r/a" or "r/a/b"
Enabled region aware placement policy, and user set the rack name which contains multiple "/" in addition to the head and tail of the string. Such as "/region/region/a" or "region/a/rack/b"

The broker will throw the following exception on onBookieRackChange

10:29:08.112 [BookKeeperClientScheduler-OrderedScheduler-0-0] ERROR org.apache.bookkeeper.client.TopologyAwareEnsemblePlacementPolicy - Unexpected exception while handling joining bookie test.bk3:3183
org.apache.bookkeeper.net.NetworkTopologyImpl$InvalidTopologyException: Invalid network topology. You cannot have a rack and a non-rack node at the same level of the network topology.
        at org.apache.bookkeeper.net.NetworkTopologyImpl.add(NetworkTopologyImpl.java:417) ~[org.apache.bookkeeper-bookkeeper-server-4.14.3.jar:4.14.3]
        at org.apache.bookkeeper.client.TopologyAwareEnsemblePlacementPolicy.handleBookiesThatJoined(TopologyAwareEnsemblePlacementPolicy.java:719) ~[org.apache.bookkeeper-bookkeeper-server-4.14.3.jar:4.14.3]
        at org.apache.bookkeeper.client.RackawareEnsemblePlacementPolicyImpl.handleBookiesThatJoined(RackawareEnsemblePlacementPolicyImpl.java:80) ~[org.apache.bookkeeper-bookkeeper-server-4.14.3.jar:4.14.3]
        at org.apache.bookkeeper.client.RackawareEnsemblePlacementPolicy.handleBookiesThatJoined(RackawareEnsemblePlacementPolicy.java:249) ~[org.apache.bookkeeper-bookkeeper-server-4.14.3.jar:4.14.3]
        at org.apache.bookkeeper.client.TopologyAwareEnsemblePlacementPolicy.onClusterChanged(TopologyAwareEnsemblePlacementPolicy.java:663) ~[org.apache.bookkeeper-bookkeeper-server-4.14.3.jar:4.14.3]
        at org.apache.bookkeeper.client.RackawareEnsemblePlacementPolicyImpl.onClusterChanged(RackawareEnsemblePlacementPolicyImpl.java:80) ~[org.apache.bookkeeper-bookkeeper-server-4.14.3.jar:4.14.3]
        at org.apache.bookkeeper.client.RackawareEnsemblePlacementPolicy.onClusterChanged(RackawareEnsemblePlacementPolicy.java:92) ~[org.apache.bookkeeper-bookkeeper-server-4.14.3.jar:4.14.3]
        at org.apache.bookkeeper.client.BookieWatcherImpl.processWritableBookiesChanged(BookieWatcherImpl.java:197) ~[org.apache.bookkeeper-bookkeeper-server-4.14.3.jar:4.14.3]
        at org.apache.bookkeeper.client.BookieWatcherImpl.lambda$initialBlockingBookieRead$1(BookieWatcherImpl.java:233) ~[org.apache.bookkeeper-bookkeeper-server-4.14.3.jar:4.14.3]
        at org.apache.bookkeeper.discover.ZKRegistrationClient$WatchTask.accept(ZKRegistrationClient.java:147) [org.apache.bookkeeper-bookkeeper-server-4.14.3.jar:4.14.3]
        at org.apache.bookkeeper.discover.ZKRegistrationClient$WatchTask.accept(ZKRegistrationClient.java:70) [org.apache.bookkeeper-bookkeeper-server-4.14.3.jar:4.14.3]
        at java.util.concurrent.CompletableFuture.uniWhenComplete(CompletableFuture.java:859) [?:?]
        at java.util.concurrent.CompletableFuture$UniWhenComplete.tryFire(CompletableFuture.java:837) [?:?]
        at java.util.concurrent.CompletableFuture$Completion.run(CompletableFuture.java:478) [?:?]
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515) [?:?]
        at java.util.concurrent.FutureTask.run(FutureTask.java:264) [?:?]
        at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304) [?:?]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) [?:?]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) [?:?]
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) [io.netty-netty-common-4.1.72.Final.jar:4.1.72.Final]
        at java.lang.Thread.run(Thread.java:834) [?:?]

Modification

Validate the rack name when setting the bookie rack info.
- For rack aware placement policy, the rack name shouldn't contains "/" in addition to the head and tail of the rack name string.
- For region aware placement policy, the rack name should only contains one "/" in addition to the head and tail of the rack name string.

Need to update docs?

doc-required

(If you need help on updating docs, create a doc issue)
no-need-doc

(Please explain why)
doc

(If this PR contains doc changes)

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Bookies.java

Anonymitaet · 2022-02-17T08:43:40Z

Hi @momo-jun can you follow up on the docs? Thanks

momo-jun · 2022-02-18T03:27:27Z

Hi @momo-jun can you follow up on the docs? Thanks

Sure. I'm on it.

Jason918

Do we have a naming pattern or doc for rack name for users?

hangc0276 · 2022-02-18T07:24:47Z

Do we have a naming pattern or doc for rack name for users?

@Jason918 There are some notices in our website and pulsar-admin info. refer to: https://pulsar.apache.org/docs/en/next/administration-isolation/

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Bookies.java

### Motivation If the rack name set in the following case: - Enabled rack aware placement policy, and user set rack name which contains "/" in addition to the head and tail of the string. Such as "/r/a" or "r/a/b" - Enabled region aware placement policy, and user set the rack name which contains multiple "/" in addition to the head and tail of the string. Such as "/region/region/a" or "region/a/rack/b" (cherry picked from commit 25408f5)

If the rack name set in the following case: - Enabled rack aware placement policy, and user set rack name which contains "/" in addition to the head and tail of the string. Such as "/r/a" or "r/a/b" - Enabled region aware placement policy, and user set the rack name which contains multiple "/" in addition to the head and tail of the string. Such as "/region/region/a" or "region/a/rack/b" (cherry picked from commit 25408f5)

If the rack name set in the following case: - Enabled rack aware placement policy, and user set rack name which contains "/" in addition to the head and tail of the string. Such as "/r/a" or "r/a/b" - Enabled region aware placement policy, and user set the rack name which contains multiple "/" in addition to the head and tail of the string. Such as "/region/region/a" or "region/a/rack/b" (cherry picked from commit 25408f5) (cherry picked from commit c7cf50a)

### Motivation If the rack name set in the following case: - Enabled rack aware placement policy, and user set rack name which contains "/" in addition to the head and tail of the string. Such as "/r/a" or "r/a/b" - Enabled region aware placement policy, and user set the rack name which contains multiple "/" in addition to the head and tail of the string. Such as "/region/region/a" or "region/a/rack/b"

Jason918 · 2022-08-01T03:51:06Z

Move release/2.7.5 label to #16850

validate rack name

7c8f42d

github-actions bot assigned hangc0276 Feb 17, 2022

hangc0276 requested review from aahmed-se, 315157973, codelipenghui, Jason918, merlimat and zymap February 17, 2022 05:42

github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Feb 17, 2022

hangc0276 added area/broker release/2.10.1 release/2.7.6 release/2.8.1 release/2.9.3 labels Feb 17, 2022

hangc0276 added this to the 2.11.0 milestone Feb 17, 2022

Jason918 reviewed Feb 17, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Bookies.java Outdated Show resolved Hide resolved

address comments

5a2887a

Jason918 approved these changes Feb 18, 2022

View reviewed changes

momo-jun mentioned this pull request Feb 18, 2022

[Doc] Add more rules for rack name validation #14374

Merged

1 task

Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Feb 21, 2022

315157973 reviewed Feb 21, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Bookies.java Outdated Show resolved Hide resolved

add rackname guide into error message

e611d40

hangc0276 requested a review from 315157973 February 25, 2022 08:46

codelipenghui approved these changes Feb 25, 2022

View reviewed changes

315157973 approved these changes Feb 25, 2022

View reviewed changes

codelipenghui merged commit 25408f5 into apache:master Feb 27, 2022

codelipenghui removed the release/2.10.1 label Mar 1, 2022

codelipenghui modified the milestones: 2.11.0, 2.10.0 Mar 1, 2022

gaoran10 added release/2.9.2 and removed release/2.9.3 labels Mar 1, 2022

gaoran10 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Mar 2, 2022

codelipenghui added release/2.8.4 and removed release/2.8.1 labels Mar 8, 2022

codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Mar 9, 2022

Jason918 added a commit to Jason918/pulsar that referenced this pull request Jul 28, 2022

cherry pick apache#14336 to branch 2.7

83ad2a3

Jason918 mentioned this pull request Jul 28, 2022

[Branch 2.7] Validate rack name #16850

Merged

3 tasks

Jason918 added release/2.7.5 and removed release/2.7.6 labels Jul 29, 2022

Jason918 added a commit to Jason918/pulsar that referenced this pull request Jul 31, 2022

cherry pick apache#14336 to branch 2.7

c8ebc76

Jason918 added a commit to Jason918/pulsar that referenced this pull request Jul 31, 2022

cherry pick apache#14336 to branch 2.7

a9a6910

Jason918 removed the release/2.7.5 label Aug 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate rack name #14336

Validate rack name #14336

hangc0276 commented Feb 17, 2022

Anonymitaet commented Feb 17, 2022

momo-jun commented Feb 18, 2022

Jason918 left a comment

hangc0276 commented Feb 18, 2022

Jason918 commented Aug 1, 2022

Validate rack name #14336

Validate rack name #14336

Conversation

hangc0276 commented Feb 17, 2022

Motivation

Modification

Anonymitaet commented Feb 17, 2022

momo-jun commented Feb 18, 2022

Jason918 left a comment

Choose a reason for hiding this comment

hangc0276 commented Feb 18, 2022

Jason918 commented Aug 1, 2022