Hi Chia-Ping > It seems we can disable the sync of idle consumers by setting > `sync.group.offsets.interval.seconds` to -1, so the fail-fast should include > sync.group.offsets.interval.seconds too. For another, maybe we should do > fail-fast for MirrorCheckpointConnector even though we don't have this KIP I don’t think we need to fail fast with `sync.group.offsets.interval.seconds` to -1, as `MirrorCheckpointConnector` runs two functionality based on offset-syncs topic that can run separately 1. Write group offset to checkpoints internal topic can be disabled with `emit.checkpoints.interval.seconds` -1 2. Schedule syncing the group offset to __consumer_offsets later can be disabled with `sync.group.offsets.interval.seconds` to -1
So technically `MirrorCheckpointConnector` can run if only one of these intervals is set to -1 however, if we want to fail fast we should check both `sync.group.offsets.interval.seconds` and `emit.checkpoints.interval.seconds` not set to -1 as this would be useless. > 2) Should we do similar fail-fast for MirrorSourceConnector if user set > custom producer configs with emit.offset-syncs.enabled=false? I assume the > producer which sending records to offset-syncs topic won't be created if > emit.offset-syncs.enabled=false This is a good point I’ll update MirrorSourceConnector’s validate method to address this. I think we should also address `offset-syncs.topic.location` and `offset-syncs.topic.replication.factor` as well as custom consumer, and admin client configs. > 3) Should we simplify the SourceRecord if emit.offset-syncs.enabled=false? > Maybe that can get a bit performance improvement. Which SourceRecord are you referring to here? Omnia > On 20 May 2024, at 16:58, Chia-Ping Tsai <chia7...@apache.org> wrote: > > Nice KIP. some minor comments/questions are listed below. > > 1) It seems we can disable the sync of idle consumers by setting > `sync.group.offsets.interval.seconds` to -1, so the fail-fast should include > sync.group.offsets.interval.seconds too. For another, maybe we should do > fail-fast for MirrorCheckpointConnector even though we don't have this KIP > > 2) Should we do similar fail-fast for MirrorSourceConnector if user set > custom producer configs with emit.offset-syncs.enabled=false? I assume the > producer which sending records to offset-syncs topic won't be created if > emit.offset-syncs.enabled=false > > 3) Should we simplify the SourceRecord if emit.offset-syncs.enabled=false? > Maybe that can get a bit performance improvement. > > Best, > Chia-Ping > > On 2024/04/08 10:03:50 Omnia Ibrahim wrote: >> Hi Chris, >> Validation method is a good call. I updated the KIP to state that the >> checkpoint connector will fail if the configs aren’t correct. And updated >> the description of the new config to explain the impact of it on checkpoint >> connector as well. >> >> If there is no any other feedback from anyone I would like to start the >> voting thread in few days. >> Thanks >> Omnia >> >>> On 8 Apr 2024, at 06:31, Chris Egerton <chr...@aiven.io.INVALID> wrote: >>> >>> Hi Omnia, >>> >>> Ah, good catch. I think failing to start the checkpoint connector if offset >>> syncs are disabled is fine. We'd probably want to do that via the >>> Connector::validate [1] method in order to be able to catch invalid configs >>> during preflight validation, but it's not necessary to get that specific in >>> the KIP (especially since we may add other checks as well). >>> >>> [1] - >>> https://kafka.apache.org/37/javadoc/org/apache/kafka/connect/connector/Connector.html#validate(java.util.Map) >>> >>> Cheers, >>> >>> Chris >>> >>> On Thu, Apr 4, 2024 at 8:07 PM Omnia Ibrahim <o.g.h.ibra...@gmail.com> >>> wrote: >>> >>>> Thanks Chris for the feedback >>>>> 1. It'd be nice to mention that increasing the max offset lag to INT_MAX >>>>> could work as a partial workaround for users on existing versions (though >>>>> of course this wouldn't prevent creation of the syncs topic). >>>> I updated the KIP >>>> >>>>> 2. Will it be illegal to disable offset syncs if other features that rely >>>>> on offset syncs are explicitly enabled in the connector config? If >>>> they're >>>>> not explicitly enabled then it should probably be fine to silently >>>> disable >>>>> them, but I'd be interested in your thoughts. >>>> The rest of the features that relays on this is controlled by >>>> emit.checkpoints.enabled (enabled by default) and >>>> sync.group.offsets.enabled (disabled by default) which are part of >>>> MirrorCheckpointConnector config not MirrorSourceConnector, I was thinking >>>> that MirrorCheckpointConnector should fail to start if >>>> emit.offset-syncs.enabled is disabled while emit.checkpoints.enabled and/or >>>> sync.group.offsets.enabled are enabled as no point of creating this >>>> connector if the main part is disabled. WDYT? >>>> >>>> Thanks >>>> Omnia >>>> >>>>> On 3 Apr 2024, at 12:45, Chris Egerton <fearthecel...@gmail.com> wrote: >>>>> >>>>> Hi Omnia, >>>>> >>>>> Thanks for the KIP! Two small things come to mind: >>>>> >>>>> 1. It'd be nice to mention that increasing the max offset lag to INT_MAX >>>>> could work as a partial workaround for users on existing versions (though >>>>> of course this wouldn't prevent creation of the syncs topic). >>>>> >>>>> 2. Will it be illegal to disable offset syncs if other features that rely >>>>> on offset syncs are explicitly enabled in the connector config? If >>>> they're >>>>> not explicitly enabled then it should probably be fine to silently >>>> disable >>>>> them, but I'd be interested in your thoughts. >>>>> >>>>> Cheers, >>>>> >>>>> Chris >>>>> >>>>> On Wed, Apr 3, 2024, 20:41 Luke Chen <show...@gmail.com> wrote: >>>>> >>>>>> Hi Omnia, >>>>>> >>>>>> Thanks for the KIP! >>>>>> It LGTM! >>>>>> But I'm not an expert of MM2, it would be good to see if there is any >>>> other >>>>>> comment from MM2 experts. >>>>>> >>>>>> Thanks. >>>>>> Luke >>>>>> >>>>>> On Thu, Mar 14, 2024 at 6:08 PM Omnia Ibrahim <o.g.h.ibra...@gmail.com> >>>>>> wrote: >>>>>> >>>>>>> Hi everyone, I would like to start a discussion thread for KIP-1031: >>>>>>> Control offset translation in MirrorSourceConnector >>>>>>> >>>>>>> >>>>>> >>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1031%3A+Control+offset+translation+in+MirrorSourceConnector >>>>>>> >>>>>>> Thanks >>>>>>> Omnia >>>>>>> >>>>>> >>>> >>>> >> >>
