> Which SourceRecord are you referring to here? I have added comments to your PR ( https://github.com/apache/kafka/pull/15999#pullrequestreview-2066823538)
in short, `sourcePartition` and `sourceOffset` are unused if emit.offset-syncs.enabled=false BTW, I'm +1 to this KIP, and I noticed my previous comments are related to code. Hence, please feel free to open votes. We can have discussion about the code later. Omnia Ibrahim <o.g.h.ibra...@gmail.com> 於 2024年5月21日 週二 下午9:10寫道： > 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 > >>>>>>> > >>>>>> > >>>> > >>>> > >> > >> > >
