> 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 I’ll have a look into the PR. Also regarding my previous comment on `sync.group.offsets.interval.seconds` we don’t need to check this if it is -1 as the only way for `sync.group.offsets.interval.seconds` or `emit.checkpoints.interval.seconds` to be -1 is if emit.checkpoints.enabled and `sync.group.offsets.enabled are both false which we check in MirrorCheckpointConnector
Anyway we can continue discussing this on the PR > 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. It was voted in few weeks ago. Omnia > On 21 May 2024, at 14:25, Chia-Ping Tsai <chia7...@gmail.com> wrote: > >> 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 >>>>>>>>> >>>>>>>> >>>>>> >>>>>> >>>> >>>> >> >>
