isaacreath commented on code in PR #360:
URL: https://github.com/apache/cassandra-sidecar/pull/360#discussion_r3544181050
##########
server/src/main/java/org/apache/cassandra/sidecar/job/OperationalJobManager.java:
##########
@@ -41,18 +43,33 @@ public class OperationalJobManager
{
protected final Logger logger = LoggerFactory.getLogger(this.getClass());
private final OperationalJobTracker jobTracker;
-
+ @Nullable
+ private final OperationalJobCoordinator coordinator;
Review Comment:
The nullable `OperationalJobCoordinator` feels like a bit of a smell to me.
It seems like the goal here is to support backwards compatibility for the
`OperationalJobManager` for users of the `OperationalJobManager` to not have to
include coordination (e.g., for existing decommission jobs), am I right about
that?
If so, I'd suggest that instead of having a null
`OperationalJobCoordinator`, we have a second implementation,
`DisabledOperationalJobCoordinator` or something similar, that fails all
coordination tasks with an error saying that coordination is not currently
supported by this sidecar instance (and maybe how to configure coordination if
the user wants to have coordinated jobs).
This way we make the coordination enabled / disabled logic part of the
behavior of the coordinator itself, as opposed to having a null dependency
here. It can also clean up all accesses to the `OperationalJobCoordinator` as
we no longer have to worry about it being null.
--
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]
