[Issue 12894] Only refresh metadata if path is already in cache after write. #12896

Jason918 · 2021-11-19T14:23:33Z

Fixes #12894

Motivation

See #12894

Modifications

Only refresh metadata if path is already in cache after write.

Verifying this change

Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is already covered by existing tests, such as (please describe tests).

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

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

Documentation

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

Need to update docs?

no-need-doc
bug fix.

Jason918 · 2021-11-19T14:29:28Z

@merlimat PTAL.
We have dozens of MetadataCacheImpl instances in AbstractMetadataStore#metadataCaches.
Each of these MetadataCacheImpl have different type of object to cache, so we can't refresh them all with the same path.

merlimat · 2021-11-19T16:27:56Z

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

@@ -331,6 +331,7 @@ public void insertionOutsideCacheWithGenericType(String provider, Supplier<Strin
        });

        String key1 = newKey();
+        objCache.get(key1).join();


This is forcing the key to be in the cache, but the test was expressly trying to check the condition in which it was not.

Changed the test to updateOutsideCacheWithGenericType for now.

I think we can implement insertionOutsideCacheWithGenericType by make a path pattern registration or add a method like accept(path) for each MetadataStore instance, which implies the metadata store could deserialization this value, and the path indeed should be cached in this metadata store.

merlimat · 2021-11-19T16:29:42Z

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/api/MetadataCache.java

+     * @param path the path of the object in the metadata store
+     * @return true if the path exists.
+     */
+    boolean existsInCache(String path);


I wouldn't add a new method in the API that we only are going to be using internally.

Two solutions:

There is already MetadataCache.getIfCached()

MetadaCache.refresh() implementation should take care of it internally.

The only reason I am not using MetadataCache.getIfCached() is it would block and wait for current load future is completed.

I am taking solution 2. It would not be "refresh" if we have not already loaded the path object, right?
Although, it may change some old behaviors.

Jason918 · 2021-11-23T01:51:50Z

@merlimat please help take a look?

Jason918 · 2021-11-23T07:10:26Z

@315157973 Here is the fix of #12909, PTAL

lhotari · 2021-11-24T08:16:09Z

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/cache/impl/MetadataCacheImpl.java

-        objCache.synchronous().invalidate(path);
-        objCache.synchronous().refresh(path);
+        // Refresh object of path if only it is cached before.
+        if (objCache.getIfPresent(path) != null) {


I'm just wondering if there's a possibility for races.

we should move this block into a dedicated method of objCache

objCache.synchronous().invalidateAndRefreshIfPresent(path)

I'm just wondering if there's a possibility for races.

No, it's ok to miss a refresh or duplicate a refresh. Worst case is you have to do an extra loading when calling get.

we should move this block into a dedicated method of objCache

objCache.synchronous().invalidateAndRefreshIfPresent(path)

It's a bit of trouble to do this, because objCache is an instance of class com.github.benmanes.caffeine.cache.AsyncCacheLoader , which is in a dependent lib.

eolivelli · 2021-11-24T09:01:04Z

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/cache/impl/MetadataCacheImpl.java

-        objCache.synchronous().invalidate(path);
-        objCache.synchronous().refresh(path);
+        // Refresh object of path if only it is cached before.
+        if (objCache.getIfPresent(path) != null) {


we should move this block into a dedicated method of objCache

objCache.synchronous().invalidateAndRefreshIfPresent(path)

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

shoothzj · 2021-11-25T00:51:29Z

comment #12788 for link

Jason918 · 2021-11-26T03:51:48Z

@eolivelli PTAL. Thanks

Jason918 · 2021-11-30T06:09:21Z

@merlimat PTAL. Thanks.

And to implement the origin "insertionOutsideCacheWithGenericType", I think we can add a path pattern registration or add a method like accept(path) for each MetadataStore instance, which implies the metadata store could deserialization this value, and the path indeed should be cached in this metadata store no matter how it's updated.
What do you think?

Review comments were addressed. This fixes an urgent issue in master branch.

eolivelli · 2021-12-01T10:04:28Z

+1

* up/master: (75 commits) [website][upgrade]feat: website upgrade / docs migration - 2.5.1 Get Started/Concepts and Architecture/Pulsar Schema (apache#13030) Fix environment variable assignment in startup scripts (apache#13025) update 2.8.x (apache#13029) [Doc] add tips for Pulsar tools (apache#13044) Suggest to use tlsPort instead of deprecated TlsEnable (apache#13039) Integration tests for function-worker rebalance and drain operations. (apache#13058) fix(functions): missing runtime set in GoInstanceConfig (apache#13031) [pulsar-admin] Add get-replicated-subscription-status command for topic (apache#12891) [Broker] Consider topics in pulsar/system namespace as system topics (apache#13050) Fix typo: correct sizeUint to sizeUnit (apache#13040) fix-12894 (apache#12896) Don't attempt to delete pending ack store unless transactions are enabled (apache#13041) [Perf] Evaluate the current protocol version once (apache#13045) Fix Issue apache#12885, Unordered consuming case in Key_Shared subscription (apache#12890) [broker]Optimize topicMaxMessageSize with topic local cache. (apache#12830) [PIP-105] Part-2 Support pluggable entry filter in Dispatcher (apache#12970) [website] Modify admin-api-topic.md document (apache#12996) add missed import (apache#13037) [metadata] Add RocksdbMetadataStore (apache#12776) [C] Add pulsar_client_subscribe_multi_topics and pulsar_client_subscribe_pattern (apache#12965) ... # Conflicts: # site2/website-next/docusaurus.config.js # site2/website-next/versioned_sidebars/version-2.6.1-sidebars.json # site2/website-next/versions.json

Co-authored-by: Jiang Haiting <jianghaiting@didichuxing.com>

Co-authored-by: Jiang Haiting <jianghaiting@didichuxing.com> (cherry picked from commit 2b939b7)

github-actions bot assigned Jason918 Nov 19, 2021

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 19, 2021

Jason918 mentioned this pull request Nov 19, 2021

A large number of exceptions occur when starting a standalone server #12895

Closed

codelipenghui approved these changes Nov 19, 2021

View reviewed changes

codelipenghui added this to the 2.10.0 milestone Nov 19, 2021

codelipenghui added the release/2.9.1 label Nov 19, 2021

codelipenghui requested a review from merlimat November 19, 2021 14:30

merlimat reviewed Nov 19, 2021

View reviewed changes

fix-12894

48863cb

Jason918 force-pushed the fix-12894 branch from 143cdde to 48863cb Compare November 20, 2021 01:55

Jason918 requested a review from merlimat November 20, 2021 02:10

Jason918 mentioned this pull request Nov 24, 2021

[Issue 12883] Fix flaky caused by message receive timeout in testBlockBrokerDispatching #12954

Merged

2 tasks

lhotari reviewed Nov 24, 2021

View reviewed changes

lhotari requested a review from eolivelli November 24, 2021 08:26

eolivelli previously requested changes Nov 24, 2021

View reviewed changes

Jason918 requested review from eolivelli and lhotari November 24, 2021 10:54

merlimat approved these changes Nov 30, 2021

View reviewed changes

lhotari approved these changes Nov 30, 2021

View reviewed changes

lhotari merged commit 2b939b7 into apache:master Nov 30, 2021

This was referenced Dec 1, 2021

Metadata Deserialization failed #12909

Closed

ISSUE-12909: Metadata Deserialization failed streamnative/pulsar-archived#3302

Closed

eolivelli added release/2.9.2 and removed release/2.9.1 labels Dec 15, 2021

fxbing pushed a commit to fxbing/pulsar that referenced this pull request Dec 19, 2021

fix-12894 (apache#12896)

20ffd99

Co-authored-by: Jiang Haiting <jianghaiting@didichuxing.com>

codelipenghui added cherry-picked/branch-2.9 Archived: 2.9 is end of life and removed cherry-picked/branch-2.9 Archived: 2.9 is end of life release/2.9.2 labels Dec 21, 2021

Jason918 mentioned this pull request Feb 8, 2022

Ensure cache is refreshed (and not just invalidated) after a store write #12788

Merged

congbobo184 added cherry-picked/branch-2.9 Archived: 2.9 is end of life release/2.9.4 labels Nov 9, 2022

congbobo184 pushed a commit to congbobo184/pulsar that referenced this pull request Nov 9, 2022

fix-12894 (apache#12896)

6ee1b0e

Co-authored-by: Jiang Haiting <jianghaiting@didichuxing.com> (cherry picked from commit 2b939b7)

congbobo184 pushed a commit that referenced this pull request Nov 9, 2022

fix-12894 (#12896)

aede60f

Co-authored-by: Jiang Haiting <jianghaiting@didichuxing.com> (cherry picked from commit 2b939b7)

congbobo184 pushed a commit that referenced this pull request Nov 26, 2022

fix-12894 (#12896)

2bbeace

Co-authored-by: Jiang Haiting <jianghaiting@didichuxing.com> (cherry picked from commit 2b939b7)

Technoboy- mentioned this pull request Dec 19, 2023

[fix][broker] Force refresh the key Technoboy-/pulsar#51

Closed

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue 12894] Only refresh metadata if path is already in cache after write. #12896

[Issue 12894] Only refresh metadata if path is already in cache after write. #12896

Jason918 commented Nov 19, 2021 •

edited

Loading

Jason918 commented Nov 19, 2021

merlimat Nov 19, 2021

Jason918 Nov 20, 2021 •

edited

Loading

merlimat Nov 19, 2021

Jason918 Nov 20, 2021 •

edited

Loading

Jason918 commented Nov 23, 2021

Jason918 commented Nov 23, 2021

lhotari Nov 24, 2021

eolivelli Nov 24, 2021

Jason918 Nov 24, 2021

Jason918 Nov 24, 2021

eolivelli Nov 24, 2021

shoothzj commented Nov 25, 2021

Jason918 commented Nov 26, 2021

Jason918 commented Nov 30, 2021

eolivelli commented Dec 1, 2021

[Issue 12894] Only refresh metadata if path is already in cache after write. #12896

[Issue 12894] Only refresh metadata if path is already in cache after write. #12896

Conversation

Jason918 commented Nov 19, 2021 • edited Loading

Motivation

Modifications

Verifying this change

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

Documentation

Jason918 commented Nov 19, 2021

merlimat Nov 19, 2021

Choose a reason for hiding this comment

Jason918 Nov 20, 2021 • edited Loading

Choose a reason for hiding this comment

merlimat Nov 19, 2021

Choose a reason for hiding this comment

Jason918 Nov 20, 2021 • edited Loading

Choose a reason for hiding this comment

Jason918 commented Nov 23, 2021

Jason918 commented Nov 23, 2021

lhotari Nov 24, 2021

Choose a reason for hiding this comment

eolivelli Nov 24, 2021

Choose a reason for hiding this comment

Jason918 Nov 24, 2021

Choose a reason for hiding this comment

Jason918 Nov 24, 2021

Choose a reason for hiding this comment

eolivelli Nov 24, 2021

Choose a reason for hiding this comment

shoothzj commented Nov 25, 2021

Jason918 commented Nov 26, 2021

Jason918 commented Nov 30, 2021

eolivelli commented Dec 1, 2021

Jason918 commented Nov 19, 2021 •

edited

Loading

Jason918 Nov 20, 2021 •

edited

Loading

Jason918 Nov 20, 2021 •

edited

Loading