Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
codelipenghui merged 2 commits into from
Nov 17, 2021
Merged

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

codelipenghui merged 2 commits into from
Nov 17, 2021

Conversation

@merlimat
Copy link
Contributor

@merlimat merlimat commented Nov 13, 2021

Motivation

When we're doing a write to the store from outside the MetadataCache, we are immediately invalidating the cache to ensure read-after-write consistency through the cache.

The only issue is that the invalidation, will not trigger a reloading of the value. Instead it is relying on the next call to cache.get() which will see the cache miss and it will load the new value into the cache.

This means that calls cache.getIfCached(), which is not triggering a cache load, will keep seeing the key as missing.

Modification

Ensure we're calling refresh on the cache to get the value automatically reloaded in background and make sure the getIfCached() will eventually return the new value, even if there are no calls to cache.get().

@merlimat merlimat added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Nov 13, 2021
@merlimat merlimat added this to the 2.10.0 milestone Nov 13, 2021
@merlimat merlimat self-assigned this Nov 13, 2021
@merlimat merlimat added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Nov 13, 2021
@apache apache deleted a comment from github-actions bot Nov 13, 2021
michaeljmarshall
michaeljmarshall approved these changes Nov 14, 2021
View reviewed changes
Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

LGTM

@codelipenghui codelipenghui merged commit 2bc4499 into apache:master Nov 17, 2021
@merlimat merlimat deleted the cache-refresh-after-write branch November 17, 2021 00:15
@massakam
Copy link
Contributor

massakam commented Nov 17, 2021

Since this change, I've been getting a lot of the following exceptions when starting the standalone server.

Nov 17, 2021 1:03:46 PM com.github.benmanes.caffeine.cache.LocalAsyncCache lambda$handleCompletion$7
WARNING: Exception thrown during asynchronous load
java.util.concurrent.CompletionException: org.apache.pulsar.metadata.api.MetadataStoreException$ContentDeserializationException: Failed to deserialize payload for key '/counters/producer-name'
        at java.util.concurrent.CompletableFuture.encodeRelay(CompletableFuture.java:326)
        at java.util.concurrent.CompletableFuture.completeRelay(CompletableFuture.java:338)
        at java.util.concurrent.CompletableFuture.uniRelay(CompletableFuture.java:925)
        at java.util.concurrent.CompletableFuture.uniCompose(CompletableFuture.java:967)
        at java.util.concurrent.CompletableFuture$UniCompose.tryFire(CompletableFuture.java:940)
        at java.util.concurrent.CompletableFuture.postComplete(CompletableFuture.java:488)
        at java.util.concurrent.CompletableFuture.complete(CompletableFuture.java:1975)
        at org.apache.pulsar.metadata.impl.ZKMetadataStore.lambda$null$7(ZKMetadataStore.java:139)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
        at java.lang.Thread.run(Thread.java:748)
Caused by: org.apache.pulsar.metadata.api.MetadataStoreException$ContentDeserializationException: Failed to deserialize payload for key '/counters/producer-name'
        at org.apache.pulsar.metadata.cache.impl.MetadataCacheImpl.lambda$readValueFromStore$0(MetadataCacheImpl.java:111)
        at java.util.concurrent.CompletableFuture.uniCompose(CompletableFuture.java:966)
        ... 8 more
Caused by: com.fasterxml.jackson.databind.exc.MismatchedInputException: No content to map due to end-of-input
 at [Source: (byte[])""; line: 1, column: 0]
        at com.fasterxml.jackson.databind.exc.MismatchedInputException.from(MismatchedInputException.java:59)
        at com.fasterxml.jackson.databind.ObjectMapper._initForReading(ObjectMapper.java:4688)
        at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4586)
        at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3643)
        at org.apache.pulsar.metadata.cache.impl.JSONMetadataSerdeSimpleType.deserialize(JSONMetadataSerdeSimpleType.java:42)
        at org.apache.pulsar.metadata.cache.impl.MetadataCacheImpl.lambda$readValueFromStore$0(MetadataCacheImpl.java:107)
        ... 9 more

@massakam massakam mentioned this pull request Nov 19, 2021
Closed
@sijie sijie mentioned this pull request Nov 19, 2021
Closed
@Jason918 Jason918 mentioned this pull request Nov 19, 2021
Closed
dlg99 pushed a commit to dlg99/pulsar that referenced this pull request Nov 23, 2021
@merlimat @dlg99
Ensure cache is refreshed (and not just invalidated) after a store wr…
3438825 
…ite (apache#12788)

### Motivation

When we're doing a write to the store from outside the `MetadataCache`, we are immediately invalidating the cache to ensure read-after-write consistency through the cache. 

The only issue is that the invalidation, will not trigger a reloading of the value. Instead it is relying on the next call to `cache.get()` which will see the cache miss and it will load the new value into the cache.

This means that calls `cache.getIfCached()`, which is not triggering a cache load, will keep seeing the key as missing.

### Modification

Ensure we're calling refresh on the cache to get the value automatically reloaded in background and make sure the `getIfCached()` will eventually return the new value, even if there are no calls to `cache.get()`.
@hezhangjian hezhangjian mentioned this pull request Nov 25, 2021
Merged
2 tasks
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request Nov 29, 2021
@merlimat @eolivelli
Ensure cache is refreshed (and not just invalidated) after a store wr…
1f21746 
…ite (apache#12788)

### Motivation

When we're doing a write to the store from outside the `MetadataCache`, we are immediately invalidating the cache to ensure read-after-write consistency through the cache. 

The only issue is that the invalidation, will not trigger a reloading of the value. Instead it is relying on the next call to `cache.get()` which will see the cache miss and it will load the new value into the cache.

This means that calls `cache.getIfCached()`, which is not triggering a cache load, will keep seeing the key as missing.

### Modification

Ensure we're calling refresh on the cache to get the value automatically reloaded in background and make sure the `getIfCached()` will eventually return the new value, even if there are no calls to `cache.get()`.
fxbing pushed a commit to fxbing/pulsar that referenced this pull request Dec 19, 2021
@merlimat
Ensure cache is refreshed (and not just invalidated) after a store wr…
e787e4b 
…ite (apache#12788)

### Motivation

When we're doing a write to the store from outside the `MetadataCache`, we are immediately invalidating the cache to ensure read-after-write consistency through the cache. 

The only issue is that the invalidation, will not trigger a reloading of the value. Instead it is relying on the next call to `cache.get()` which will see the cache miss and it will load the new value into the cache.

This means that calls `cache.getIfCached()`, which is not triggering a cache load, will keep seeing the key as missing.

### Modification

Ensure we're calling refresh on the cache to get the value automatically reloaded in background and make sure the `getIfCached()` will eventually return the new value, even if there are no calls to `cache.get()`.
@lhotari
Copy link
Member

lhotari commented Feb 7, 2022

@merlimat Is there a need to backport this fix to branch-2.8 and branch-2.9 ? /cc @codelipenghui @Jason918 @mattisonchao

There seem to be several metadata inconsistency issues in 2.8.x+ , please check the comments in #12555 (comment) .

@lhotari
Copy link
Member

lhotari commented Feb 7, 2022 •
edited
Loading

@merlimat I have a question regarding the solution in this PR: why is the cached entry always invalidated or refreshed? After the change has been made to Zookeeper, is there a specific reason why the same entry cannot be put input the Metadata cache? Since the cache entry is type of https://github.com/apache/pulsar/blob/master/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/api/CacheGetResult.java , it already contains the reference to the Stat object instance and therefore could resolve a possible cache put conflict?

@Jason918
Copy link
Contributor

Jason918 commented Feb 8, 2022

Is there a need to backport this fix to branch-2.8 and branch-2.9 ?

I think this PR is a "better to have".
If you do, please remember to backport it with #12896

@congbobo184 congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 9, 2022
congbobo184 pushed a commit to congbobo184/pulsar that referenced this pull request Nov 9, 2022
@merlimat
Ensure cache is refreshed (and not just invalidated) after a store wr…
498a0a7 
…ite (apache#12788)

### Motivation

When we're doing a write to the store from outside the `MetadataCache`, we are immediately invalidating the cache to ensure read-after-write consistency through the cache.

The only issue is that the invalidation, will not trigger a reloading of the value. Instead it is relying on the next call to `cache.get()` which will see the cache miss and it will load the new value into the cache.

This means that calls `cache.getIfCached()`, which is not triggering a cache load, will keep seeing the key as missing.

### Modification

Ensure we're calling refresh on the cache to get the value automatically reloaded in background and make sure the `getIfCached()` will eventually return the new value, even if there are no calls to `cache.get()`.

(cherry picked from commit 2bc4499)
lhotari pushed a commit that referenced this pull request Nov 9, 2022
@merlimat @lhotari
Ensure cache is refreshed (and not just invalidated) after a store wr…
88b1a96 
…ite (#12788)

### Motivation

When we're doing a write to the store from outside the `MetadataCache`, we are immediately invalidating the cache to ensure read-after-write consistency through the cache.

The only issue is that the invalidation, will not trigger a reloading of the value. Instead it is relying on the next call to `cache.get()` which will see the cache miss and it will load the new value into the cache.

This means that calls `cache.getIfCached()`, which is not triggering a cache load, will keep seeing the key as missing.

### Modification

Ensure we're calling refresh on the cache to get the value automatically reloaded in background and make sure the `getIfCached()` will eventually return the new value, even if there are no calls to `cache.get()`.

(cherry picked from commit 2bc4499)
congbobo184 pushed a commit that referenced this pull request Nov 26, 2022
@merlimat
Ensure cache is refreshed (and not just invalidated) after a store wr…
dc1ce08 
…ite (#12788)

### Motivation

When we're doing a write to the store from outside the `MetadataCache`, we are immediately invalidating the cache to ensure read-after-write consistency through the cache.

The only issue is that the invalidation, will not trigger a reloading of the value. Instead it is relying on the next call to `cache.get()` which will see the cache miss and it will load the new value into the cache.

This means that calls `cache.getIfCached()`, which is not triggering a cache load, will keep seeing the key as missing.

### Modification

Ensure we're calling refresh on the cache to get the value automatically reloaded in background and make sure the `getIfCached()` will eventually return the new value, even if there are no calls to `cache.get()`.

(cherry picked from commit 2bc4499)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Jason918 Jason918 Jason918 approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@michaeljmarshall michaeljmarshall michaeljmarshall approved these changes

@rdhabalia rdhabalia Awaiting requested review from rdhabalia

@hangc0276 hangc0276 Awaiting requested review from hangc0276

@BewareMyPower BewareMyPower Awaiting requested review from BewareMyPower

Assignees

@merlimat merlimat

Labels

cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-not-needed Your PR changes do not impact docs release/2.9.4 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages

Projects

None yet

Milestone

2.10.0

Development

Successfully merging this pull request may close these issues.

7 participants

@merlimat @massakam @lhotari @Jason918 @codelipenghui @michaeljmarshall @congbobo184