recursive persistent ZK watch #105
Conversation
…ate.copyWith(prs) as setPerReplicaStates is only available in fs/branch_9x and not fs/branch_9_2
| State existing = states.get(newState.replica); | ||
| if (existing == null || existing.version < newState.version) { | ||
| LinkedHashMap<String, State> copy = new LinkedHashMap<>(states.size()); | ||
| states.forEachEntry(copy::put); |
There was a problem hiding this comment.
Can we just use copy ctor LinkedHashMap<String, State> copy = new LinkedHashMap<>(states); here? :)
| new WrappedSimpleMap<>(copy), | ||
| czxid); // child node's czxid is same as state.json pzxid | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Maybe good to include in javadoc that if the insertion is not successful, then null will be returned, as such is not that obvious to the caller
| @@ -41,7 +41,7 @@ public static PerReplicaStates fetch( | |||
| if (current != null) { | |||
| Stat stat = zkClient.exists(current.path, null, true); | |||
| if (stat == null) return new PerReplicaStates(path, 0, Collections.emptyList()); | |||
| if (current.cversion == stat.getCversion()) return current; // not modifiedZkStateReaderTest | |||
| if (current.pzxid == stat.getCversion()) return current; // not modifiedZkStateReaderTest | |||
There was a problem hiding this comment.
should we use stat.getPzxid() here
| @@ -41,7 +41,7 @@ public static PerReplicaStates fetch( | |||
| if (current != null) { | |||
| Stat stat = zkClient.exists(current.path, null, true); | |||
| if (stat == null) return new PerReplicaStates(path, 0, Collections.emptyList()); | |||
| if (current.cversion == stat.getCversion()) return current; // not modifiedZkStateReaderTest | |||
| if (current.pzxid == stat.getCversion()) return current; // not modifiedZkStateReaderTest | |||
| } | |||
| Stat stat = new Stat(); | |||
| List<String> children = zkClient.getChildren(path, null, stat, true); | |||
There was a problem hiding this comment.
next line should be stat.getPzxid() too?
| } | ||
| prs = prs.insert(newState, stat.getCzxid()); | ||
| if (prs == null) { | ||
| // something went wrong |
There was a problem hiding this comment.
This seems to get triggered for every core during collection creation. as preceding create event on the collection invokes line 407 an fetch the collection state with the PerReplicateStates already. Therefore all following PRS path create event will actually find identical entries and failed (https://github.com/cowpaths/fullstory-solr/pull/105/files#diff-0bd8a828302915c525c8df3e8cccdc9881ebad121359c0dbc8374b8b72995669R151, which both version numbers are the same)
| path, | ||
| new WrappedSimpleMap<>(copy), | ||
| czxid); // child node's czxid is same as state.json pzxid | ||
| } |
There was a problem hiding this comment.
Probably want to add a case
} else if (newState.asString.equals(existing.asString)) {
return this;
}
here to avoid unnecessary fetch. With the current design, the existing state can be the same as the new state
| private DocCollection currentState; | ||
|
|
||
| Watcher persistentWatcher; |
There was a problem hiding this comment.
init of this uses double-checked locking later on https://github.com/cowpaths/fullstory-solr/pull/105/files#diff-9b1918d641f78a8c245306b47ffd7a4554a12771ed4cb42293a411537fe7b54dR1563-R1568. Perhaps add volatile? :)
| public void removePersistentWatch() { | ||
| try { | ||
| log.info("removed persistent watch for {}", coll); | ||
| zkClient.removePersistentWatch( |
There was a problem hiding this comment.
add a null check to persistentWatcher just to be safe?
|
Seeing accumulation of persistent watches, we probably want to remove the watch at here as well |
|
|
||
| refreshAndWatch(event.getType()); | ||
| } | ||
| public void process(WatchedEvent event) {} |
There was a problem hiding this comment.
Perhaps we should move https://github.com/cowpaths/fullstory-solr/pull/105/files#diff-9b1918d641f78a8c245306b47ffd7a4554a12771ed4cb42293a411537fe7b54dR396-R510 to here.
It seems more appropriate for watcher to handle watched events instead of watch itself doing it. It is confusing that this watcher seems to do nothing while the actual watch handling is defined elsewhere
| } | ||
|
|
||
| refreshAndWatchChildren(); |
There was a problem hiding this comment.
We probably do not need to watch the children here, since this watcher does nothing. As far as i understand the design, we install a persistent recursive watch that watch both the state.json and PRS entries (recommend move such handling logic out of StatefulCollectionWatch and into here).
And it seems we are doing a one-off fetch on all children to construct the PerReplicaStates here? if that's the case, can we just do a zkClient.getChildren without passing any watcher so it wouldn't register any extra watch with the ZK
| log.trace("Replica: {} delete, force fetch", path); | ||
| } | ||
| // a replica got removed. we can't get the pzxid reliably. fetch everything | ||
| prs = PerReplicaStatesFetcher.fetch(collectionPath, zkClient, null); |
There was a problem hiding this comment.
Found race condition here. It could reach this even though it's not replica deletion (and it seems to happen frequently).
For example, adding a replica:
- creates PRS entry (create node event)
core_node4:0:D - create node event
core_node4:1:R - delete node vent
core_node4:0D
However step 2 and step 3 can happen concurrently, 2nd notification should come in first based on the watch guarantee, however the SolrZkClient delegates to the watcher in async manner , that means before 2nd operation actually update the collection state in line 504, step 3 can already read the collection state, which has the old PRS states in step 1, this would make the logic to incorrectly identified that as replica removal
No description provided.