Re: [DISCUSS] KIP-1031: Control offset translation in MirrorSourceConnector
> 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 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 於 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 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 >> 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
Re: [DISCUSS] KIP-1031: Control offset translation in MirrorSourceConnector
> 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 於 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 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 > 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 > >>> 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
Re: [DISCUSS] KIP-1031: Control offset translation in MirrorSourceConnector
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 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 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 >>> 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 wrote: > > Hi Omnia, > > Thanks for the KIP! Two small things
Re: [DISCUSS] KIP-1031: Control offset translation in MirrorSourceConnector
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 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 > > 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 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 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 > 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 > > > > >> > >> > >
Re: [DISCUSS] KIP-1031: Control offset translation in MirrorSourceConnector
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 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 > 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 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 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 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 > >> >>
Re: [DISCUSS] KIP-1031: Control offset translation in MirrorSourceConnector
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 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 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 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 > >> 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 > >>> > >> > >
Re: [DISCUSS] KIP-1031: Control offset translation in MirrorSourceConnector
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 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 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 >> 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 >>> >>
Re: [DISCUSS] KIP-1031: Control offset translation in MirrorSourceConnector
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 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 > 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 > > >
Re: [DISCUSS] KIP-1031: Control offset translation in MirrorSourceConnector
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 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 >
[DISCUSS] KIP-1031: Control offset translation in MirrorSourceConnector
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
