[Issue 10860][pulsar-metadata] Ensure metadata cache consistency across brokers #10862
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
June 7, 2021 20:35
1. During create we should add a watch for the path in zookeeper. Without this we will not be notified if the znode is changed on another brokers 2. similarly when deleting, the cache should be invalidated. But we shouldn't add an entry to the cache. This could get added again on some other broker. In that case we need to go a fetch the entry from the zookeeper. Adding an empty entry to the cache prevents that.
|
@merlimat : Can you help review this change. |
merlimat
reviewed
Jun 8, 2021
There was a problem hiding this comment.
Nice catch here!
The only doubt I have here is if it makes sense to do the fix in the ZK store implementation instead.
The reason is that since this is mostly related with the watch semantics which is very peculiar of ZK. Also, since we're now on ZK 3.6, we can also switch to use persistent watches (eg: setting a watch on an entire subtree and not having to re-set it each time).
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/cache/impl/MetadataCacheImpl.java
Show resolved
Hide resolved
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/cache/impl/MetadataCacheImpl.java
Outdated
Show resolved
Hide resolved
|
@sursingh thanks for your contribution. For this PR, do we need to update docs? |
|
/pulsarbot run-failure-checks |
|
@Anonymitaet This is only a bugfix. We don't need to update the docs for this. |
Also add a small delay in test to allow notifications to propagate to other caches. Without this the tests are occasionally failing
jerrypeng
approved these changes
Jun 8, 2021
merlimat
approved these changes
Jun 8, 2021
codelipenghui
pushed a commit
that referenced
this pull request
Jun 11, 2021
…ss brokers (#10862) * Added metadata cache test to simulate multi broker cache * fix create and delete ops on cache 1. During create we should add a watch for the path in zookeeper. Without this we will not be notified if the znode is changed on another brokers 2. similarly when deleting, the cache should be invalidated. But we shouldn't add an entry to the cache. This could get added again on some other broker. In that case we need to go a fetch the entry from the zookeeper. Adding an empty entry to the cache prevents that. * Address review comments Also add a small delay in test to allow notifications to propagate to other caches. Without this the tests are occasionally failing Co-authored-by: Surinder Singh <surinders@splunk.com> (cherry picked from commit 798b34f)
eolivelli
pushed a commit
to datastax/pulsar
that referenced
this pull request
Jun 14, 2021
…ss brokers (apache#10862) * Added metadata cache test to simulate multi broker cache * fix create and delete ops on cache 1. During create we should add a watch for the path in zookeeper. Without this we will not be notified if the znode is changed on another brokers 2. similarly when deleting, the cache should be invalidated. But we shouldn't add an entry to the cache. This could get added again on some other broker. In that case we need to go a fetch the entry from the zookeeper. Adding an empty entry to the cache prevents that. * Address review comments Also add a small delay in test to allow notifications to propagate to other caches. Without this the tests are occasionally failing Co-authored-by: Surinder Singh <surinders@splunk.com> (cherry picked from commit 798b34f) (cherry picked from commit d280fad)
yangl
pushed a commit
to yangl/pulsar
that referenced
this pull request
Jun 23, 2021
…ss brokers (apache#10862) * Added metadata cache test to simulate multi broker cache * fix create and delete ops on cache 1. During create we should add a watch for the path in zookeeper. Without this we will not be notified if the znode is changed on another brokers 2. similarly when deleting, the cache should be invalidated. But we shouldn't add an entry to the cache. This could get added again on some other broker. In that case we need to go a fetch the entry from the zookeeper. Adding an empty entry to the cache prevents that. * Address review comments Also add a small delay in test to allow notifications to propagate to other caches. Without this the tests are occasionally failing Co-authored-by: Surinder Singh <surinders@splunk.com>
This was referenced Aug 5, 2021
bharanic-dev
pushed a commit
to bharanic-dev/pulsar
that referenced
this pull request
Mar 18, 2022
…ss brokers (apache#10862) * Added metadata cache test to simulate multi broker cache * fix create and delete ops on cache 1. During create we should add a watch for the path in zookeeper. Without this we will not be notified if the znode is changed on another brokers 2. similarly when deleting, the cache should be invalidated. But we shouldn't add an entry to the cache. This could get added again on some other broker. In that case we need to go a fetch the entry from the zookeeper. Adding an empty entry to the cache prevents that. * Address review comments Also add a small delay in test to allow notifications to propagate to other caches. Without this the tests are occasionally failing Co-authored-by: Surinder Singh <surinders@splunk.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #10860
Motivation
When doing operations on multiple brokers, the state is not always consistent. Sometimes the operations don't seem to get replicated to other brokers in the cluster. The queries from the brokers returns different results. See #10860 for details
Modifications
While creating a new object, we add the object to cache. However we are not adding a watch on the corresponding path in zookeeper. So when other brokers change/delete the object, the broker that added the object is not notified. With this change we will do a get(), which will add a watch on the corresponding path. This will also add the object to the cache.
Similarly while deleting, we are adding an empty object to the cache. Any local request will now return a
object not founderror, without needing to go the zookeeper. However if the object get re-added on any other broker, we will not be notified. We will continue to returnobject not founderror, even though an instance of the object now exist. This change remove the addition of empty object to the cache. If a request is received for the object, we will do a get from the zookeeper and if it still doesn't exist, an empty object will get added. Also a watch will get added for the corresponding path. This will ensure that cache will get update the object is again added by some other broker.Verifying this change
Does this pull request potentially affect one of the following parts:
If
yeswas chosen, please highlight the changes