keith-turner commented on code in PR #5732:
URL: https://github.com/apache/accumulo/pull/5732#discussion_r2201742020
##########
server/base/src/main/java/org/apache/accumulo/server/manager/LiveTServerSet.java:
##########
@@ -211,10 +213,23 @@ static class TServerInfo {
// The set of entries in zookeeper without locks, and the first time each
was noticed
private final Map<ServiceLockPath,Long> locklessServers = new HashMap<>();
- public LiveTServerSet(ServerContext context, Listener cback) {
- this.cback = cback;
+ public LiveTServerSet(ServerContext context) {
+ this.cback = new AtomicReference<>(null);
this.context = context;
- this.context.getZooCache().addZooCacheWatcher(this);
+ }
+
+ public void setCback(Listener cback) {
Review Comment:
I looked into the test and I understand what is going on, the test is not
currently calling startListeningForTabletServerChanges() and trying to call it
complicates the mocking in the test. I think we could avoid merging the two
methods in this PR and maybe do it in another.
Not sure if this is workable, the code in LiveTServerSetTest seems like its
trying to test a single method: `LiveTServerSet.find()`. It would probably
be best to refactor that single method to make it more testable by itselft w/o
requiring an entire LiveTServerSet object to be created. So I think this test
could be improved and that would make merging these methods easier. I suspect
creating that entire object to test that single method add a lot of uneeded
complexity.
--
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]
