Skip to content

Fix invalid rack name cause bookie join rack failed - #13683

Merged
315157973 merged 5 commits into
apache:masterfrom
hangc0276:chenhang/fix_rackname_invalid
Jan 16, 2022
Merged

Fix invalid rack name cause bookie join rack failed#13683
315157973 merged 5 commits into
apache:masterfrom
hangc0276:chenhang/fix_rackname_invalid

Conversation

@hangc0276

Copy link
Copy Markdown
Contributor

Motivation

When user set rack name to / or empty string "", it will cause all matched bookies join target rack failed and throw the following exception

java.lang.StringIndexOutOfBoundsException: String index out of range: -1
        at java.lang.String.substring(String.java:1841) ~[?:?]
        at org.apache.bookkeeper.net.NetworkTopologyImpl$InnerNode.getNextAncestorName(NetworkTopologyImpl.java:144) ~[io.streamnative-bookkeeper-server-4.14.3.1.jar:4.14.3.1]
        at org.apache.bookkeeper.net.NetworkTopologyImpl$InnerNode.add(NetworkTopologyImpl.java:180) ~[io.streamnative-bookkeeper-server-4.14.3.1.jar:4.14.3.1]
        at org.apache.bookkeeper.net.NetworkTopologyImpl.add(NetworkTopologyImpl.java:425) ~[io.streamnative-bookkeeper-server-4.14.3.1.jar:4.14.3.1]
        at org.apache.bookkeeper.client.TopologyAwareEnsemblePlacementPolicy.handleBookiesThatJoined(TopologyAwareEnsemblePlacementPolicy.java:717) ~[io.streamnative-bookkeeper-server-4.14.3.1.jar:4.14.3.1]
        at org.apache.bookkeeper.client.RackawareEnsemblePlacementPolicyImpl.handleBookiesThatJoined(RackawareEnsemblePlacementPolicyImpl.java:80) ~[io.streamnative-bookkeeper-server-4.14.3.1.jar:4.14.3.1]
        at org.apache.bookkeeper.client.RackawareEnsemblePlacementPolicy.handleBookiesThatJoined(RackawareEnsemblePlacementPolicy.java:249) ~[io.streamnative-bookkeeper-server-4.14.3.1.jar:4.14.3.1]
        at org.apache.bookkeeper.client.TopologyAwareEnsemblePlacementPolicy.onClusterChanged(TopologyAwareEnsemblePlacementPolicy.java:663) ~[io.streamnative-bookkeeper-server-4.14.3.1.jar:4.14.3.1]
        at org.apache.bookkeeper.client.RackawareEnsemblePlacementPolicyImpl.onClusterChanged(RackawareEnsemblePlacementPolicyImpl.java:80) ~[io.streamnative-bookkeeper-server-4.14.3.1.jar:4.14.3.1]
        at org.apache.bookkeeper.client.RackawareEnsemblePlacementPolicy.onClusterChanged(RackawareEnsemblePlacementPolicy.java:92) ~[io.streamnative-bookkeeper-server-4.14.3.1.jar:4.14.3.1]
        at org.apache.bookkeeper.client.BookieWatcherImpl.processWritableBookiesChanged(BookieWatcherImpl.java:197) ~[io.streamnative-bookkeeper-server-4.14.3.1.jar:4.14.3.1]
        at org.apache.bookkeeper.client.BookieWatcherImpl.lambda$initialBlockingBookieRead$1(BookieWatcherImpl.java:233) ~[io.streamnative-bookkeeper-server-4.14.3.1.jar:4.14.3.1]
        at org.apache.bookkeeper.discover.ZKRegistrationClient$WatchTask.accept(ZKRegistrationClient.java:147) [io.streamnative-bookkeeper-server-4.14.3.1.jar:4.14.3.1]
        at org.apache.bookkeeper.discover.ZKRegistrationClient$WatchTask.accept(ZKRegistrationClient.java:70) [io.streamnative-bookkeeper-server-4.14.3.1.jar:4.14.3.1]
        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:829) [?:?]

Which will lead ledger create failed and throw ManagedLedgerException: Not enough non-faulty bookies available

Modification

  1. Add rack name checker on bookie rack info set.
  2. For the has been set rack name, we add a checker in BookieRackAffinityMapping, and return null to fallback to /default-rack or /default-region/default-rack
  3. I will add a checker in BookKeeper client side to avoid StringIndexOutOfBoundsException

Documentation

Check the box below or label this PR directly (if you have committer privilege).

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)

@github-actions github-actions Bot added the doc-required Your PR changes impact docs and you will update later. label Jan 9, 2022
@hangc0276 hangc0276 added this to the 2.10.0 milestone Jan 9, 2022
@gaoran10

gaoran10 commented Jan 9, 2022

Copy link
Copy Markdown
Contributor

Could we add a check for the param rack at bookie cmd?

@hangc0276

Copy link
Copy Markdown
Contributor Author

Could we add a check for the param rack at bookie cmd?

@gaoran10 Thanks for you review, I added the check on cmdBookie side.

Comment thread pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdBookies.java Outdated
hangc0276 and others added 2 commits January 11, 2022 15:05
…ckawareness/BookieRackAffinityMapping.java

Co-authored-by: lipenghui <penghui@apache.org>
…CmdBookies.java

Co-authored-by: lipenghui <penghui@apache.org>

@gaoran10 gaoran10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lgtm

@315157973
315157973 merged commit af761e2 into apache:master Jan 16, 2022
@gaozhangmin gaozhangmin mentioned this pull request Jan 17, 2022
3 tasks
codelipenghui pushed a commit that referenced this pull request Jan 18, 2022
* Fix invalid rackname cause bookie join rack failed

(cherry picked from commit af761e2)
@codelipenghui codelipenghui added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Jan 18, 2022
codelipenghui pushed a commit that referenced this pull request Jan 18, 2022
* Fix invalid rackname cause bookie join rack failed

(cherry picked from commit af761e2)
@codelipenghui codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Jan 18, 2022
@Anonymitaet

Copy link
Copy Markdown
Member

pulsar-admin bookies is not shown on https://pulsar.apache.org/tools/pulsar-admin/2.10.0-SNAPSHOT/. @urfreespace will investigate this issue. If pulsar-admin bookies can not be shown on https://pulsar.apache.org/tools/pulsar-admin/2.10.0-SNAPSHOT/, I'll add the docs to https://pulsar.apache.org/docs/en/next/reference-cli-tools/#bookie

@Anonymitaet 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 Jan 21, 2022
@lhotari

lhotari commented Feb 10, 2022

Copy link
Copy Markdown
Member

I didn't find a way to cherry-pick this cleanly to branch-2.7. I'll move this to 2.7.6. Please open a separate PR for branch-2.7.

@hangc0276

Copy link
Copy Markdown
Contributor Author

I didn't find a way to cherry-pick this cleanly to branch-2.7. I'll move this to 2.7.6. Please open a separate PR for branch-2.7.

@lhotari I will create another PR for branch-2.7

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

Jason918 commented Aug 1, 2022

Copy link
Copy Markdown
Contributor

Move release/2.7.5 label to #16845

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/broker cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-complete Your PR changes impact docs and the related docs have been already added. release/2.8.3 release/2.9.2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants