[fix][broker] Invalidate metadata children cache after key deleted

[Shawyeok]

• This fixes Failed to create subscription #13489
• This fixes BadVersionException , unable to consume topic, failing to reconnect #14779

Motivation

There is a chance if a topic reassign to a broker that owned this topic once, consumer reconnect will failure with BadVersionException, here's detail: #14779 (comment)

Reproduce steps:

1. a topic deleted in broker1
2. recreate the topic in broker2, start a consumer with it
3. broker crash or bundle split, topic reassign to broker1, by now consumer will reconnect failure

Modifications

Cleanup metadata children cache if managed-ledger key deleted

Verifying this change

• Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

• Dependencies (add or upgrade a dependency)
• The public API
• The schema
• The default values of configurations
• The threading model
• The binary protocol
• The REST endpoints
• The admin CLI options
• The metrics
• Anything that affects deployment

Documentation

• doc-not-needed

Matching PR in forked repository

PR in forked repository: Shawyeok#7

[Shawyeok]

/pulsarbot rerun-failure-checks

[tisonkun]

Besides, I remember that ZK cannot remove recursively at a batch but iterating from children to parent. How the cache won't be invalidated by its children getting deleted?

Anyway, invalidate cache eagerly won't introduce correctness issue.

[Shawyeok]

Besides, I remember that ZK cannot remove recursively at a batch but iterating from children to parent. How the cache won't be invalidated by its children getting deleted?

Anyway, invalidate cache eagerly won't introduce correctness issue.

In the scenario described in #14779 (comment), node1 does not have a watcher associated with the newly created znode cursor. As a result, the mechanism to invalidate the parent key children cache will not work.

[codecov-commenter]

Codecov Report

Merging #20363 (4e9086f) into master (1e664b7) will increase coverage by 36.01%.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##             master   #20363       +/-   ##
=============================================
+ Coverage     36.93%   72.95%   +36.01%     
- Complexity    12032    31889    +19857     
=============================================
  Files          1687     1864      +177     
  Lines        128761   138403     +9642     
  Branches      14002    15185     +1183     
=============================================
+ Hits          47553   100965    +53412     
+ Misses        74932    29427    -45505     
- Partials       6276     8011     +1735     

Flag	Coverage Δ
inttests	24.17% <100.00%> (-0.16%)	⬇️
systests	24.90% <100.00%> (-0.10%)	⬇️
unittests	72.22% <100.00%> (+40.28%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...he/pulsar/metadata/impl/AbstractMetadataStore.java	85.38% <100.00%> (+32.67%)	⬆️

... and 1430 files with indirect coverage changes

[tisonkun]

Thanks for your explanation! Now I know that no children cause the cache be empty and return empty cursor even if there is some. And this patch is valid even for defensive programming (delete node should always invalid cache).

Although, if the cursor is a child of the topic znode, won't putInternal invalid childrenCache so that the cursor be properly loaded?

I'd appreciate it if you can provide a znode view of execution steps to demonstrate the exception.

[Shawyeok]

I'd appreciate it if you can provide a znode view of execution steps to demonstrate the exception.

Try me break down into two steps to explain how this error could be produced in #14779:

1. When topic back to node1, it will only initialize the managed-ledger without initializing any cursors, due to cache consistency
   pseudo call stack:

MetaStore#getCursor
ManagedLedgerImpl#initializeCursors
MetaStore#asyncUpdateLedgerIds
ManagedLedgerImpl#asyncCreateLedger
ManagedLedgerImpl#initializeBookKeeper
MetaStore#getManagedLedgerInfo
ManagedLedgerImpl#initialize

pulsar/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java

Lines 570 to 572 in 1e664b7

	if (consumers.isEmpty()) {
	callback.initializeComplete();
	return;

2. When a consumer reconnects to node1, the broker only checks if the cursor has been loaded into memory
   

   pulsar/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java

   Lines 981 to 982 in 1e664b7

   	ManagedCursor cachedCursor = cursors.get(cursorName);
   	if (cachedCursor != null) {

pseudo call stack:

AbstractMetadataStore#put
MetaStore#asyncUpdateCursorInfo
ManagedCursorImpl#persistPositionMetaStore
ManagedCursorImpl#switchToNewLedger
ManagedCursorImpl#createNewMetadataLedger
ManagedCursorImpl#initialize
ManagedLedger#asyncOpenCursor
PersistentTopic#getDurableSubscription
PersistentTopic#internalSubscribe
PersistentTopic#subscribe
ServerCnx#handleSubscribe

BTW, broker will try create a cursor znode which already exists in zk, so it'll receive NODEEXISTS.

Here's NODEEXISTS how to be handled, a BADVERSION:

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

Lines 244 to 246 in 3f2978d

	} else if (code == Code.NODEEXISTS) {
	// We're emulating a request to create node, so the version is invalid
	op.getFuture().completeExceptionally(getException(Code.BADVERSION, op.getPath()));

In summary, this is how I observed the occurrence of BadVersionException.

[tisonkun]

After a discussion with @Shawyeok I learned that it's about two broker node that:

1. Broker 1 has cache for topic znode.
2. Broker 1 switches to a different topic, delete the topic znode but forget to clear the children cache.
3. Broker 2 create the topic znode and cursor child znode
4. Broker 1 switches back to the original topic. It doesn't set watches or perform put cursor znode by itself so the child cache is still empty (not uninit).

[tisonkun]

Merging...

Thanks for your contribution and detailed investigation @Shawyeok!

[Technoboy-]

@Shawyeok Could you add a test for this patch?

[Shawyeok]

@Shawyeok Could you add a test for this patch?

Sure, I'll do it later.

[Shawyeok]

This patch was aimed to fix #14779, I reproduced in my environment which is based on the 2.8.1 codebase. After following @Technoboy's suggestion to write a reproduction test in the master branch, I realized that #11198 uses zk persistent watches, which should also fix #14779.

However, in theory, there is a chance that the new cursor watcher event is not triggered before MetadataStore#getChildren gets called on other brokers. In such cases, this patch also works effectively.

[michaeljmarshall]

As discussed on the mailing list https://lists.apache.org/thread/w4jzk27qhtosgsz7l9bmhf1t7o9mxjhp, there is no plan to release 2.9.6, so I am going to remove the release/2.9.6 label