TODO Cleanup - #6509
Conversation
Amemeda
changed the title
if (retryCountDownTimer.isExpired()) {
// TODO exception used for timeout is inconsistent
throw new TimedOutException(
"Failed to find servers to process scans before timeout was exceeded.");
}Does anyone know why this exception from |
No idea |
| if (retryCountDownTimer.isExpired()) { | ||
| // TODO exception used for timeout is inconsistent | ||
| throw new TimedOutException( | ||
| "Failed to find servers to process scans before timeout was exceeded."); | ||
| } |
There was a problem hiding this comment.
Removed this TODO for now. The exception used here seems consistent with what is happening: TimedOutException because the retryCountDownTimer has expired. If anything, the message may not be accurate description of why the timeout happened.
I can add it back if the todo should remain, or if a different exception/message should be used here, I can replace.
| zkLockChecker = memoize(() -> { | ||
| // make this use its own ZooSession and ZooCache, because this is used by the | ||
| // tablet location cache, which is a static singleton reused by multiple clients | ||
| // so, it can't rely on being able to continue to use the same client's ZooCache, | ||
| // because that client could be closed, and its ZooSession also closed | ||
| // this needs to be fixed; TODO https://github.com/apache/accumulo/issues/2301 | ||
| var zk = info.getZooKeeperSupplier(ZookeeperLockChecker.class.getSimpleName(), | ||
| ZooUtil.getRoot(getInstanceID())).get(); | ||
| return new ZookeeperLockChecker(new ZooCache(zk, Set.of(Constants.ZTSERVERS))); |
There was a problem hiding this comment.
Attaching comment from issue discussing the relevance of this TODO. It looks like it is no longer relevant but could use @ctubbsii opinion.
#2699 (comment)
There was a problem hiding this comment.
I haven't looked over the whole PR, but I think it's correct that this comment is no longer applicable on the main branch, as it is currently written, since the location cache is not shared across clients anymore after #5282.
However, the code is still creating its own ZooCache for some reason, instead of using the context-specific one. So either:
- that is no longer necessary and it can use the one already in the ClientContext, and the comment can be removed after that refactor, or
- it is still necessary because the lock checker is leaking into threads that can survive after closing the ClientContext, in which case the comment needs to be updated to reflect the current justification.
I don't know which scenario is the case.
…ThrottledBalancerProblemReporter.java Add Table to the obscured Key Extent output Co-authored-by: Dom G. <domgarguilo@apache.org>
4 participants
ThrottledBalancerProblemReporter.java. Added this logic onto existing .stream in the logger that logs 10 outstanding migrations. Related to Jira issue 2938ClientContext.java. Github issue Explore refactoring TabletLocator code #2301 mentioned in the TODO has been closed for over a year.TabletServerBatchReaderIterator.java, the exception used here does not seem inconsistent since the retryCountDownTimer has expired.Value.javaJira issue 2509 mentioned in the TODO has been resolved as Not A Problem.Removes 4 TODOs , partially addresses #2699