kfaraz commented on code in PR #13074:
URL: https://github.com/apache/druid/pull/13074#discussion_r974880978
##########
server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java:
##########
@@ -973,6 +973,14 @@ public List<? extends CoordinatorDuty> getDuties()
{
return duties;
}
+
+ @Override
+ public String toString()
+ {
+ return "DutiesRunnable{" +
+ "dutiesRunnableAlias='" + dutiesRunnableAlias + '\'' +
+ '}';
+ }
Review Comment:
Oh, yeah, the `VisibleForTesting` is ubiquitous 😅
It would be good to pull out `DutiesRunnable`. Right now, it seems to be
directly using pretty much all the fields that `DruidCoordinator` contains.
That's probably why it is still hanging around here and why it is not a
_static_ inner class either.
The preferable way to do this would be for `DruidCoordinator` to expose a
bunch of methods that update the state of segmentManager and other fields that
`DutiesRunnable` needs to access. And the `DutiesRunnable` constructor just
gets the `DruidCoordinator` instance. `DruidCoordinator` already exposes other
such utility methods such as `moveSegment()` or `markSegmentAsUnused()` which
are used by the actual duties themselves.
Let me know if this approach makes sense. We can get it done in a follow-up
PR.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
