frankvicky commented on code in PR #16767:
URL: https://github.com/apache/kafka/pull/16767#discussion_r1704242756
##########
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorCheckpointConnector.java:
##########
@@ -136,10 +137,14 @@ public List<Map<String, String>> taskConfigs(int
maxTasks) {
// if the replication is disabled, known consumer group is empty, or
checkpoint emission is
// disabled by setting 'emit.checkpoints.enabled' to false, the
interval of checkpoint emission
// will be negative and no 'MirrorCheckpointTask' will be created
- if (!config.enabled() || knownConsumerGroups.isEmpty()
- || config.emitCheckpointsInterval().isNegative()) {
+ if (!config.enabled() ||
config.emitCheckpointsInterval().isNegative()) {
return Collections.emptyList();
+ } else if (knownConsumerGroups.isEmpty()) {
Review Comment:
Hi @C0urante
Thanks for the suggestion. 😸
I think leveraging `null` to represent the initial state is very insightful.
Although we have to deal with possible NPEs, it is still worthwhile compared
to the effort of unnecessarily retriggering or shutting down tasks
unintentionally.
If we do in this way, I believe we need to add a comment to explain the
different meanings between null and an empty list.
And about the log statement at L96~97, I totally agree, fair enough. 👍🏼
--
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]
