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 > >>>>> > >>>> > >> > >> > >
