[fix] [meta]Switch to the metadata store thread after zk operation #20303

poorbarcode · 2023-05-11T09:49:21Z

Motivation

#5358 makes the thread switch to the metadata store thread after the ZK operation.
#13043 Add the feature batch operation for ZKMetadataStore, but after the batch operation is done, the thread is not switched back.

Now, if we call put /a, it will use the event thread of the ZK client after the operation; but if we call put /a/b/c(the node /a is not exists), it will use the metadata store thread after the operation.

Modifications

Switch to the metadata store thread after the batch operation.
If there has any exception, error handling also uses the metadata store thread

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository:

x

pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreTest.java

lhotari · 2023-05-12T11:36:49Z

@poorbarcode codes this also resolve the deadlock issue reported in #20148 ?

poorbarcode · 2023-05-12T14:19:41Z

@lhotari

codes this also resolve the deadlock issue reported in #20148 ?

Only solve the error ZooKeeper session reconnection timeout. Notifying session is lost. the deadlock still exists(The locked thread switches from zk client event thread to metadata store thread).

I will try to fix #20148 later. Thank you for reminding me. You are really a careful man.

nodece

LGTM

michaeljmarshall · 2023-05-17T18:47:14Z

Now, if we call put /a, it will use the event thread of the ZK client after the operation; but if we call put /a/b/c(the node /a is not exists), it will use the metadata store thread after the operation.

What is the consequence of using different threads? Did you see out of order events?

EDIT: looks like the consequence is this error ZooKeeper session reconnection timeout. Notifying session is lost.

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java

poorbarcode · 2023-05-18T03:32:52Z

rebase master

…20303)

…20303) (cherry picked from commit 35d9612)

…pache#20303) (cherry picked from commit 35d9612) (cherry picked from commit 32e1463)

…ata when doing lookup (#21063) Motivation: If we set `allowAutoTopicCreationType` to `PARTITIONED`, the flow of the create topic progress is like the below: 1. `Client-side`: Lookup topic to get partitioned topic metadata to create a producer. 1. `Broker-side`: Create partitioned topic metadata. 1. `Broker-side`: response `{"partitions":3}`. 1. `Client-side`: Create separate connections for each partition of the topic. 1. `Broker-side`: Receive 3 connect requests and create 3 partition-topics. In the `step 2` above, the flow of the progress is like the below: 1. Check the policy of topic auto-creation( the policy is `{allowAutoTopicCreationType=PARTITIONED, defaultNumPartitions=3}` ) 1. Check the partitioned topic metadata already exists. 1. Try to create the partitioned topic metadata if it does not exist. 1. If created failed by the partitioned topic metadata already exists( maybe another broker is also creating now), read partitioned topic metadata from the metadata store and respond to the client. There is a race condition that makes the client get non-partitioned metadata of the topic: | time | `broker-1` | `broker-2` | | --- | --- | --- | | 1 | get policy: `PARTITIONED, 3` | get policy: `PARTITIONED, 3` | | 2 | check the partitioned topic metadata already exists | Check the partitioned topic metadata already exists | | 3 | Partitioned topic metadata does not exist, the metadata cache will cache an empty optional for the path | Partitioned topic metadata does not exist, the metadata cache will cache an empty optional for the path | | 4 | | succeed create the partitioned topic metadata | | 5 | Receive a ZK node changed event to invalidate the cache of the partitioned topic metadata | | 6 | Creating the metadata failed due to it already exists | | 7 | Read the partitioned topic metadata again | If `step-5` is executed later than `step-7`, `broker-1` will get an empty optional from the cache of the partitioned topic metadata and respond non-partitioned metadata to the client. **What thing would make the `step-5` is executed later than `step-7`?** Provide a scenario: Such as the issue that the PR #20303 fixed, it makes `zk operation` and `zk node changed notifications` executed in different threads: `main-thread of ZK client` and `metadata store thread`. Therefore, the mechanism of the lookup partitioned topic metadata is fragile and we need to optimize it. Modifications: Before reading the partitioned topic metadata again, refresh the cache first.

…ata when doing lookup (#21063) Motivation: If we set `allowAutoTopicCreationType` to `PARTITIONED`, the flow of the create topic progress is like the below: 1. `Client-side`: Lookup topic to get partitioned topic metadata to create a producer. 1. `Broker-side`: Create partitioned topic metadata. 1. `Broker-side`: response `{"partitions":3}`. 1. `Client-side`: Create separate connections for each partition of the topic. 1. `Broker-side`: Receive 3 connect requests and create 3 partition-topics. In the `step 2` above, the flow of the progress is like the below: 1. Check the policy of topic auto-creation( the policy is `{allowAutoTopicCreationType=PARTITIONED, defaultNumPartitions=3}` ) 1. Check the partitioned topic metadata already exists. 1. Try to create the partitioned topic metadata if it does not exist. 1. If created failed by the partitioned topic metadata already exists( maybe another broker is also creating now), read partitioned topic metadata from the metadata store and respond to the client. There is a race condition that makes the client get non-partitioned metadata of the topic: | time | `broker-1` | `broker-2` | | --- | --- | --- | | 1 | get policy: `PARTITIONED, 3` | get policy: `PARTITIONED, 3` | | 2 | check the partitioned topic metadata already exists | Check the partitioned topic metadata already exists | | 3 | Partitioned topic metadata does not exist, the metadata cache will cache an empty optional for the path | Partitioned topic metadata does not exist, the metadata cache will cache an empty optional for the path | | 4 | | succeed create the partitioned topic metadata | | 5 | Receive a ZK node changed event to invalidate the cache of the partitioned topic metadata | | 6 | Creating the metadata failed due to it already exists | | 7 | Read the partitioned topic metadata again | If `step-5` is executed later than `step-7`, `broker-1` will get an empty optional from the cache of the partitioned topic metadata and respond non-partitioned metadata to the client. **What thing would make the `step-5` is executed later than `step-7`?** Provide a scenario: Such as the issue that the PR #20303 fixed, it makes `zk operation` and `zk node changed notifications` executed in different threads: `main-thread of ZK client` and `metadata store thread`. Therefore, the mechanism of the lookup partitioned topic metadata is fragile and we need to optimize it. Modifications: Before reading the partitioned topic metadata again, refresh the cache first. (cherry picked from commit d099ac4)

…ata when doing lookup (apache#21063) Motivation: If we set `allowAutoTopicCreationType` to `PARTITIONED`, the flow of the create topic progress is like the below: 1. `Client-side`: Lookup topic to get partitioned topic metadata to create a producer. 1. `Broker-side`: Create partitioned topic metadata. 1. `Broker-side`: response `{"partitions":3}`. 1. `Client-side`: Create separate connections for each partition of the topic. 1. `Broker-side`: Receive 3 connect requests and create 3 partition-topics. In the `step 2` above, the flow of the progress is like the below: 1. Check the policy of topic auto-creation( the policy is `{allowAutoTopicCreationType=PARTITIONED, defaultNumPartitions=3}` ) 1. Check the partitioned topic metadata already exists. 1. Try to create the partitioned topic metadata if it does not exist. 1. If created failed by the partitioned topic metadata already exists( maybe another broker is also creating now), read partitioned topic metadata from the metadata store and respond to the client. There is a race condition that makes the client get non-partitioned metadata of the topic: | time | `broker-1` | `broker-2` | | --- | --- | --- | | 1 | get policy: `PARTITIONED, 3` | get policy: `PARTITIONED, 3` | | 2 | check the partitioned topic metadata already exists | Check the partitioned topic metadata already exists | | 3 | Partitioned topic metadata does not exist, the metadata cache will cache an empty optional for the path | Partitioned topic metadata does not exist, the metadata cache will cache an empty optional for the path | | 4 | | succeed create the partitioned topic metadata | | 5 | Receive a ZK node changed event to invalidate the cache of the partitioned topic metadata | | 6 | Creating the metadata failed due to it already exists | | 7 | Read the partitioned topic metadata again | If `step-5` is executed later than `step-7`, `broker-1` will get an empty optional from the cache of the partitioned topic metadata and respond non-partitioned metadata to the client. **What thing would make the `step-5` is executed later than `step-7`?** Provide a scenario: Such as the issue that the PR apache#20303 fixed, it makes `zk operation` and `zk node changed notifications` executed in different threads: `main-thread of ZK client` and `metadata store thread`. Therefore, the mechanism of the lookup partitioned topic metadata is fragile and we need to optimize it. Modifications: Before reading the partitioned topic metadata again, refresh the cache first. (cherry picked from commit d099ac4)

…ata when doing lookup (apache#21063) Motivation: If we set `allowAutoTopicCreationType` to `PARTITIONED`, the flow of the create topic progress is like the below: 1. `Client-side`: Lookup topic to get partitioned topic metadata to create a producer. 1. `Broker-side`: Create partitioned topic metadata. 1. `Broker-side`: response `{"partitions":3}`. 1. `Client-side`: Create separate connections for each partition of the topic. 1. `Broker-side`: Receive 3 connect requests and create 3 partition-topics. In the `step 2` above, the flow of the progress is like the below: 1. Check the policy of topic auto-creation( the policy is `{allowAutoTopicCreationType=PARTITIONED, defaultNumPartitions=3}` ) 1. Check the partitioned topic metadata already exists. 1. Try to create the partitioned topic metadata if it does not exist. 1. If created failed by the partitioned topic metadata already exists( maybe another broker is also creating now), read partitioned topic metadata from the metadata store and respond to the client. There is a race condition that makes the client get non-partitioned metadata of the topic: | time | `broker-1` | `broker-2` | | --- | --- | --- | | 1 | get policy: `PARTITIONED, 3` | get policy: `PARTITIONED, 3` | | 2 | check the partitioned topic metadata already exists | Check the partitioned topic metadata already exists | | 3 | Partitioned topic metadata does not exist, the metadata cache will cache an empty optional for the path | Partitioned topic metadata does not exist, the metadata cache will cache an empty optional for the path | | 4 | | succeed create the partitioned topic metadata | | 5 | Receive a ZK node changed event to invalidate the cache of the partitioned topic metadata | | 6 | Creating the metadata failed due to it already exists | | 7 | Read the partitioned topic metadata again | If `step-5` is executed later than `step-7`, `broker-1` will get an empty optional from the cache of the partitioned topic metadata and respond non-partitioned metadata to the client. **What thing would make the `step-5` is executed later than `step-7`?** Provide a scenario: Such as the issue that the PR apache#20303 fixed, it makes `zk operation` and `zk node changed notifications` executed in different threads: `main-thread of ZK client` and `metadata store thread`. Therefore, the mechanism of the lookup partitioned topic metadata is fragile and we need to optimize it. Modifications: Before reading the partitioned topic metadata again, refresh the cache first. (cherry picked from commit d099ac4) (cherry picked from commit 2e534d2)

github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 11, 2023

poorbarcode self-assigned this May 11, 2023

poorbarcode added release/2.10.5 release/2.11.2 release/3.0.1 area/metadata labels May 11, 2023

poorbarcode added this to the 3.1.0 milestone May 11, 2023

nodece reviewed May 12, 2023

View reviewed changes

pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreTest.java Outdated Show resolved Hide resolved

nodece reviewed May 12, 2023

View reviewed changes

pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreTest.java Show resolved Hide resolved

poorbarcode requested a review from nodece May 12, 2023 14:04

nodece approved these changes May 15, 2023

View reviewed changes

nodece requested review from merlimat, eolivelli, codelipenghui and michaeljmarshall May 17, 2023 02:30

michaeljmarshall reviewed May 17, 2023

View reviewed changes

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java Outdated Show resolved Hide resolved

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java Outdated Show resolved Hide resolved

poorbarcode force-pushed the fix/meta_store_thread_switch branch from 0ede03e to e498f17 Compare May 18, 2023 03:32

poorbarcode requested a review from michaeljmarshall May 18, 2023 04:18

poorbarcode added 3 commits May 30, 2023 16:11

[fix] [meta]Switch to the metadatastore thread after zk operation

357e1f3

improve the test

96f49e2

address comments

305cf9c

Technoboy- force-pushed the fix/meta_store_thread_switch branch from cbf91d1 to 305cf9c Compare May 30, 2023 08:11

Technoboy- approved these changes May 30, 2023

View reviewed changes

Technoboy- merged commit 35d9612 into apache:master May 30, 2023

Technoboy- pushed a commit that referenced this pull request May 30, 2023

[fix] [meta]Switch to the metadata store thread after zk operation (#…

9dba33b

…20303)

Technoboy- added the cherry-picked/branch-2.11 label May 30, 2023

poorbarcode added the cherry-picked/branch-3.0 label May 30, 2023

poorbarcode added a commit that referenced this pull request May 30, 2023

[fix] [meta]Switch to the metadata store thread after zk operation (#…

b97a1b4

…20303) (cherry picked from commit 35d9612)

poorbarcode mentioned this pull request May 31, 2023

[fix][test] Fix test testThreadSwitchOfZkMetadataStore #20446

Merged

4 tasks

tisonkun mentioned this pull request Jun 15, 2023

[Bug] Broker gets block while creating topic using IsolatedBookieEnsemblePlacement in missing cache #20148

Closed

2 tasks

liangyepianzhou pushed a commit that referenced this pull request Jul 7, 2023

[fix] [meta]Switch to the metadata store thread after zk operation (#…

32e1463

…20303) (cherry picked from commit 35d9612)

liangyepianzhou added the cherry-picked/branch-2.10 label Jul 7, 2023

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jul 17, 2023

[fix] [meta]Switch to the metadata store thread after zk operation (a…

9a9b135

…pache#20303) (cherry picked from commit 35d9612) (cherry picked from commit 32e1463)

poorbarcode mentioned this pull request Aug 24, 2023

[improve] [broker] Improve cache handling for partitioned topic metadata when doing lookup #21063

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] [meta]Switch to the metadata store thread after zk operation #20303

[fix] [meta]Switch to the metadata store thread after zk operation #20303

poorbarcode commented May 11, 2023 •

edited

Loading

lhotari commented May 12, 2023

poorbarcode commented May 12, 2023

nodece left a comment

michaeljmarshall commented May 17, 2023 •

edited

Loading

poorbarcode commented May 18, 2023

[fix] [meta]Switch to the metadata store thread after zk operation #20303

[fix] [meta]Switch to the metadata store thread after zk operation #20303

Conversation

poorbarcode commented May 11, 2023 • edited Loading

Motivation

Modifications

Documentation

Matching PR in forked repository

lhotari commented May 12, 2023

poorbarcode commented May 12, 2023

nodece left a comment

Choose a reason for hiding this comment

michaeljmarshall commented May 17, 2023 • edited Loading

poorbarcode commented May 18, 2023

poorbarcode commented May 11, 2023 •

edited

Loading

michaeljmarshall commented May 17, 2023 •

edited

Loading