ctubbsii commented on code in PR #5292:
URL: https://github.com/apache/accumulo/pull/5292#discussion_r1935931372
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ZookeeperLockChecker.java:
##########
@@ -46,7 +48,14 @@ public boolean isLockHeld(String tserver, String session) {
@Override
public void invalidateCache(String tserver) {
- zc.clear(root + "/" + tserver);
+ String serverPath = root + "/" + tserver;
Review Comment:
I think this was not quite what I had in mind with #5259 . The thing I was
identifying in that was that in #5008 (main branch only), there was an exact
match on the tserver lock path for the clear. Now, it's rewritten as a
Predicate to account for the fact that the tserver could be in multiple
resource groups, and the exact paths aren't precisely known by the caller. That
is fine, but the two parts where it is less precise are in the
`.startsWith(zkRoot + "/tservers")`, which would also match on things like
`zkRoot + "/tservers-backups"` or `zkRoot + "/tserversAndOtherThings". So, that
startsWith should end with a slash. And then the other part that is less
precise is the `.contains(tserver)`. We don't know what value `tserver` is at
this point in the code. If it's something like "accumulo" and zkRoot is
"/accumulo", then it's going to always match. Hosts typically have port
numbers, though, so this isn't very likely. However, we can also have zkRoot
paths that contain things that
look like port numbers (in theory). This might be a bit contrived, but the
point is that it could be possible. So, if we want to be as precise as it was
before, we need to do something like `endsWith` or a more precise `contains`
call that perhaps looks something like `contains("/" + tserver + "/zlock-")`.
The worst case scenario is that we clear something from ZooCache that we
didn't need to, so this may not be that important. The main point of the ticket
was just that in the main branch, the check is less precise after #5008, and
could be made a little more precise, so it's not clearing stuff that isn't
necessary to clear.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
