Fixes #1220 - Only watch and cache values actually in ZooKeeper and p… #2
Conversation
… and properly remove all table config watchers on table delete
|
|
||
| private volatile boolean closed = false; | ||
|
|
||
| public static final String[] extraConfigs = {"table.split.endrow.size.max", |
There was a problem hiding this comment.
this is too fragile. Perhaps we can iterate over the constants instead (Property enum)? I am not too sure how we can distinguish between a property that may not exist when a watcher is created vs one that always will exist.
There was a problem hiding this comment.
I agree. I believe that I might have found only a subset of the list of things that potentially may have watches put on them but are never really created. I will investigate this.
This code change assumes a node exists until proven otherwise so it is safe. Watchers will be put on the same Znodes the way they usually are if they are not in the list of "extraConfigs". "extraConfigs" should probably be called "possiblyExistingAndWatchableConfigs" ..
| log.trace("Deleting the table config " + tableConfigPath + "/" + tableConfig); | ||
| org.apache.zookeeper.data.Stat stat = | ||
| zoo.getZooKeeper().exists(tableConfigPath + "/" + tableConfig, false); | ||
| zoo.getZooKeeper().delete(tableConfigPath + "/" + tableConfig, stat.getVersion()); |
There was a problem hiding this comment.
why do a delete here if we are doing a recursive delete below?
There was a problem hiding this comment.
Thanks Ivan. I took that call out and retested. It is not needed.
| Stat stat; | ||
| if (zPath.endsWith(possiblyWatchedConfig)) { | ||
| try { | ||
| stat = zooKeeper.exists(zPath, false); |
There was a problem hiding this comment.
make a comment that this will remove the watcher?
There was a problem hiding this comment.
It doesn't remove watcher. It checks for node existence with placing a watch on the node. We are only checking or existence and nothing else here.
| } else { | ||
| try { | ||
| data = zooKeeper.getData(zPath, watcher, stat); | ||
| data = zooKeeper.getData(zPath, watched ? watcher : null, stat); |
There was a problem hiding this comment.
So in this case if we are not watched (meaning the property is either not in the list of does not exist), then we will not watch it at all?
There was a problem hiding this comment.
The default and expected value of the boolean 'watched' is true. That is what we used to do for every node.
… and properly remove all table config watchers on table delete
jzgithub1
force-pushed
the
accumulo-1220
branch
from
53be534 to
4f74ca3
Compare
| } | ||
|
|
||
| switch (event.getType()) { | ||
| case NodeDataChanged: |
There was a problem hiding this comment.
If the NodeChildrenChanged block is not valid for NodeDataChanged, then you could simply move this case below the NodeChildrenChanged block and avoid the check in the if close on line 162
| private final HashMap<String,byte[]> cache; | ||
| private final HashMap<String,ZcStat> statCache; | ||
| private final HashMap<String,List<String>> childrenCache; | ||
| private final HashMap<String,Boolean> znodeExists = new HashMap<>(); |
There was a problem hiding this comment.
You appear to be using this map as a set (i.e. you never put(path, false)). Why not simply make this a set?
| switch (event.getType()) { | ||
| case NodeDataChanged: | ||
| case NodeChildrenChanged: | ||
| if (event.getPath().endsWith("conf") |
There was a problem hiding this comment.
is checking endsWith("conf") specific enough for table conf directories? Is there any other path that ends with conf?
| if (znodeExists.containsKey(zPath)) | ||
| return watched; | ||
|
|
||
| if (zPath.contains("tserver.") || zPath.contains("table.")) { |
There was a problem hiding this comment.
This check seems odd to me.... are we looking for only configuration properties here?
There was a problem hiding this comment.
We are all set now. Let me know if you want me to make a pull request.
Added missing permission checks to ClientServiceHandler Thrift API methods that retrieve the system/namespace/table properties, versioned properties, or configuration. The system user will be able to call these methods. To get the system properties / configuration, the caller will need SystemPermission.SYSTEM. To get the namespace properties / configuration, the caller will need NamespacePermission.ALTER_NAMESPACE on the namespace. To get the table properties / configuration, the caller will need TablePermission.ALTER_TABLE on the table. One side effect of this change is that when cloning a table, if the user is not the table creator, then the user will need either TablePermission.ALTER_TABLE on the source table or NamespacePermission.ALTER_NAMESPACE on the namespace.
2 participants
…roperly remove all table config watchers on table delete