You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/09/21 22:22:38 UTC
[GitHub] [pulsar] merlimat opened a new pull request #12120: Avoid trying to refresh MetadataCaches when we're disconnected from ZK
merlimat opened a new pull request #12120:
URL: https://github.com/apache/pulsar/pull/12120
### Motivation
We're using a 5min TTL for entries stored in the metadata cache. After that time a refresh is triggered in background.
This is done to prevent an entry that could have gone out of sync to remain stale forever.
As an enhancement, if we know that we are not connected to metadata service, we should not attempt the refresh because it would certainly fail. Instead, we wait until we're reconnected back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #12120: Avoid trying to refresh MetadataCaches when we're disconnected from ZK
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #12120:
URL: https://github.com/apache/pulsar/pull/12120#discussion_r714934723
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java
##########
@@ -65,6 +67,9 @@
private final AsyncLoadingCache<String, Boolean> existsCache;
private final CopyOnWriteArrayList<MetadataCacheImpl<?>> metadataCaches = new CopyOnWriteArrayList<>();
+ @Getter
+ private boolean isConnected = true;
Review comment:
It's fair to want to avoid the overhead. My one concern is that this could have undefined, or at least unexpected, behavior because we'll be dependent on the JIT compiler's implementation. This stack overflow has some answers that describe my concerns: https://stackoverflow.com/a/16472105. Although, I'll admit that the answers a bit old and I haven't researched to see if the comments are still valid for later versions of java.
What do you think about adding a debug log statement or a metric to track the usage of old metadata values? Either would make it easier to observe the behavior.
Thanks for adding the comment to the variable. I think that will be helpful for future reference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #12120: Avoid trying to refresh MetadataCaches when we're disconnected from ZK
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #12120:
URL: https://github.com/apache/pulsar/pull/12120#discussion_r714856498
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java
##########
@@ -246,6 +261,18 @@ public void registerSessionListener(Consumer<SessionEvent> listener) {
}
protected void receivedSessionEvent(SessionEvent event) {
+ switch (event) {
Review comment:
Yes, added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #12120: Avoid trying to refresh MetadataCaches when we're disconnected from ZK
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #12120:
URL: https://github.com/apache/pulsar/pull/12120#discussion_r714961002
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java
##########
@@ -65,6 +67,9 @@
private final AsyncLoadingCache<String, Boolean> existsCache;
private final CopyOnWriteArrayList<MetadataCacheImpl<?>> metadataCaches = new CopyOnWriteArrayList<>();
+ @Getter
+ private boolean isConnected = true;
Review comment:
Thank you for the explanation. I hadn't come across that design decision for stats. I agree that a slightly stale value is acceptable here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat merged pull request #12120: Avoid trying to refresh MetadataCaches when we're disconnected from ZK
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #12120:
URL: https://github.com/apache/pulsar/pull/12120
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #12120: Avoid trying to refresh MetadataCaches when we're disconnected from ZK
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #12120:
URL: https://github.com/apache/pulsar/pull/12120#discussion_r714749120
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java
##########
@@ -246,6 +261,18 @@ public void registerSessionListener(Consumer<SessionEvent> listener) {
}
protected void receivedSessionEvent(SessionEvent event) {
+ switch (event) {
Review comment:
this code is the same we added recently.
what about adding an utility method in SessionEvent
```
public boolean isConnected() {
switch (event) {
case Reconnected:
case SessionReestablished:
isConnected = true;
break;
case ConnectionLost:
case SessionLost:
isConnected = false;
break;
}
return isConnected;
}
```
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java
##########
@@ -65,6 +67,9 @@
private final AsyncLoadingCache<String, Boolean> existsCache;
private final CopyOnWriteArrayList<MetadataCacheImpl<?>> metadataCaches = new CopyOnWriteArrayList<>();
+ @Getter
+ private boolean isConnected = true;
Review comment:
@merlimat what about adding a comment ? otherwise in the future someone else could send a patch to add `volatile`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #12120: Avoid trying to refresh MetadataCaches when we're disconnected from ZK
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #12120:
URL: https://github.com/apache/pulsar/pull/12120#discussion_r713619581
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java
##########
@@ -65,6 +67,9 @@
private final AsyncLoadingCache<String, Boolean> existsCache;
private final CopyOnWriteArrayList<MetadataCacheImpl<?>> metadataCaches = new CopyOnWriteArrayList<>();
+ @Getter
+ private boolean isConnected = true;
Review comment:
I was trying to avoid extra overhead here since we don't need exact consistent behavior here. This is a best-effort tentative to avoid the refreshes, to avoid printing the cache reloading exceptions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #12120: Avoid trying to refresh MetadataCaches when we're disconnected from ZK
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #12120:
URL: https://github.com/apache/pulsar/pull/12120#discussion_r713569623
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java
##########
@@ -65,6 +67,9 @@
private final AsyncLoadingCache<String, Boolean> existsCache;
private final CopyOnWriteArrayList<MetadataCacheImpl<?>> metadataCaches = new CopyOnWriteArrayList<>();
+ @Getter
+ private boolean isConnected = true;
Review comment:
Should this have the `volatile` keyword? It seems like it will be written to from the `ZKSessionWatcher` thread and then read from the cache's threads.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #12120: Avoid trying to refresh MetadataCaches when we're disconnected from ZK
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #12120:
URL: https://github.com/apache/pulsar/pull/12120#discussion_r714949186
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java
##########
@@ -65,6 +67,9 @@
private final AsyncLoadingCache<String, Boolean> existsCache;
private final CopyOnWriteArrayList<MetadataCacheImpl<?>> metadataCaches = new CopyOnWriteArrayList<>();
+ @Getter
+ private boolean isConnected = true;
Review comment:
What volatile gives you is that if you modify that variable (in a particular sequence), you're guaranteed to immediately see the change from a different thread (in same consistent order in which they were changed).
If it's not volatile, the other thread will see that change some time later. Typically that corresponds to when either thread writing or reading is encountering a memory barrier (on *any* other variable). In practice it means that the flag will be visible in a tiny amount of time.
We're extensively using this approach when reading stats and similars, so that we can expose values without paying the price each time.
For this particular usage, I mentioned that this is best effort. The reason is that when we detect that the ZK connection is broken, it's already "late". The detection is based on running a probe every 2sec to validate ZK connectivity, so when the prob fails, we already got some time in which we thought we were connected but we're really not anymore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org