handle zkCache failure: invalidate cache and zk-getData failure #377
Conversation
| @@ -166,6 +166,23 @@ private void invalidateExists(String path) { | |||
| existsCache.invalidate(path); | |||
| } | |||
|
|
|||
| public void asyncInvalidate(String path) { | |||
| if (scheduledExecutor != null) { | |||
There was a problem hiding this comment.
Why the executors could be null?
because in case of ZookeeperCacheLoader we depend on ForkJoinPool.
Also, it can also possible if someone pass null executor in constructor: ZooKeeperCache(ZooKeeper zkSession, OrderedSafeExecutor executor, ScheduledExecutorService scheduledExecutor)
There was a problem hiding this comment.
Isn't it better to check this in the constructor then?
There was a problem hiding this comment.
Actually we have multiple tests which passes null for executor to test specific scenario. Also,
- we do handle null executor in ZkCache at multiple places
- so, if caller may pass null executor then I think it should be handled.
There was a problem hiding this comment.
ok.. we can add assertion in constructor for non null executor .
rdhabalia
force-pushed
the
handle_zk
branch
4 times, most recently
from
3969dc7 to
38779db
Compare
| @@ -95,6 +95,8 @@ | |||
| private LocalZooKeeperConnectionService localZooKeeperConnectionProvider; | |||
| private final ScheduledExecutorService executor = Executors.newScheduledThreadPool(20, | |||
| new DefaultThreadFactory("pulsar")); | |||
| private final ScheduledExecutorService cacheExecutor = Executors.newScheduledThreadPool(10, | |||
| new DefaultThreadFactory("cache-callback")); | |||
There was a problem hiding this comment.
Isn't it better to be specicif? zk-cache-callback instead of cache-callback?
| @@ -166,6 +166,23 @@ private void invalidateExists(String path) { | |||
| existsCache.invalidate(path); | |||
| } | |||
|
|
|||
| public void asyncInvalidate(String path) { | |||
| if (scheduledExecutor != null) { | |||
There was a problem hiding this comment.
Isn't it better to check this in the constructor then?
| @@ -95,6 +95,8 @@ | |||
| private LocalZooKeeperConnectionService localZooKeeperConnectionProvider; | |||
| private final ScheduledExecutorService executor = Executors.newScheduledThreadPool(20, | |||
| new DefaultThreadFactory("pulsar")); | |||
| private final ScheduledExecutorService cacheExecutor = Executors.newScheduledThreadPool(10, | |||
There was a problem hiding this comment.
just a thought - can we make the pools size configurable and put all constants in one place - for all pools cacheExecutor, scheduledExecutorScheduler and orderedExecutor
There was a problem hiding this comment.
we may configure number of netty-io and executor threads but I think we can create an issue and should address into different PR.
|
@merlimat updated with testcases. can you please review when you get a chance. |
* handle zkCache failure: invalidate cache and zk-getData failure * introduce separate executor to serve zkcache callback * add executor to discovery service * remove testing npe * add assetion on zkCache executor parameter and update tests * update executor thread in testcase for intermittent failure
Motivation
Due to some reason if broker lose Global-Zk session then
ZooKeeperCacheshould handle failure while getting data from lost zkSession.Modifications
futurefrom the cache so, next time when call comes then ZKCache can load fresh data from reconnected ZKSession.Result
It helps
ZooKeeperCacheto reload zk-node data if it fails earlier with zksession issue.