Thanks Sushanth. I updated KIP to remove the module dependency, create a helper interface in share-coordinator, and add a new metric. Please take a look.
Thanks, Murali On Mon, Jul 13, 2026 at 5:39 PM Sushant Mahajan <[email protected]> wrote: > Hi, > Thanks for the response. > > Regarding > > sm01 - I would prefer not a add group coordinator dependency on share > coordinator. Thats why we created coordinator-common. > > Could we instead create a helper interface in share-coordinator to access > the relevant configs and pass the impl from BrokerServer (which has access > to relevant group config managers) to the share coordinator service? > > sm04 - yes, lets add that one. > > Rest of the answers are good. > > Regards, > Sushant Mahajan > > On Fri, 10 Jul 2026, 22:55 Muralidhar Basani via dev, < > [email protected]> > wrote: > > > Thanks again Sushant for the nice review. > > > > sm01 > > > > True. share-coordinator has no compile dependency on group-coordinator. > The > > KIP adds implementation project(':group-coordinator') to the > > share-coordinator build. There is no cyclic dep. > > > > ShareGroupConfigProvider gets a new accessor > > snapshotUpdateBytesPerSnapshotOrDefault(groupId, brokerDefault) > > ShareCoordinatorShard.Builder gets a new setter > > withShareGroupConfigProvider(.....) and ShareCoordinatorService.Builder > > gets the same setter. > > With these, BrokerServer can now construct the provider and pass it in. > The > > shard can then communicate with the provider on both the write path > > (generateShareStateRecord) and the replay path (handleShareSnapshot), > > falling back to the broker-level default (when the group has no > override). > > > > sm02 > > > > Good point. A default value is not distinguishable from an explicit value > > via getLong() alone. > > > > I have updated KIP Public interfaces -> deprecated config section. > > 1. If share.coordinator.snapshot.update.bytes.per.snapshot is in > > originals() -> bytes mode. > > 2. Else if share.coordinator.snapshot.update.records.per.snapshot is in > > originals() -> records mode (deprecated path). > > 3. Else (neither explicitly set) -> bytes mode at the 20 mb default. > > 4. If both are in originals() -> bytes mode wins and a warn is logged. > > > > This resolution happens once at startup in ShareCoordinatorConfig, not > > per-record. > > > > sm03 > > > > Cluster-roll backward compatibility > > > > During a rolling upgrade, yes brokers run mixed versions. Each broker's > > shard independently decides snapshot cadence, from its own local config. > A > > newer broker replaying records written by an older broker (or other way > > around) interprets them identically. So cadence may differ in the > mid-roll, > > but there is no on-disk format change and no coordination required I > see. A > > broker that hasn't yet had the new config set, will simply use the 20 MB > > default. I have updated Compatibility section in the kip. > > > > sm04 > > > > Currently the records-based didn't have any. A new metric > > bytes-since-snapshot (per __share_group_state partition) could be useful. > > Let me know if this is good, or any other suggestion? > > > > sm05 > > > > The threshold is measured on uncompressed serialized record bytes. There > is > > a little note in the kip mentioning that "Compression is not accounted > for; > > the threshold is on uncompressed record bytes." > > Actually share.coordinator.state.topic.compression.codec defaults to > NONE. > > So by default on-disk bytes equal the measured bytes; So it will not > affect > > the 20mb limit, in default deploymnt. > > > > Thanks, > > Murali > > > > On Thu, Jul 9, 2026 at 7:25 PM Sushant Mahajan <[email protected]> > > wrote: > > > > > Hi, > > > Went over the new byte based approach. > > > > > > Few questions: > > > > > > sm01 - currently share coord module does not depend on the group > > > coordinator. Could you detail out how the new group level config will > be > > > injected in the share coordinator? > > > > > > sm02 - since we have default defined as 20MB, how to differentiate > > between > > > older update count set vs byte limit set? The kip mentions if set byte > > > level will be honored. How to determine set vs default? > > > > > > sm03 - could you drop a line about backward compatibility in case of > > > cluster roll? > > > > > > sm04 - should we also add any new metrics? > > > > > > sm05 - do measurements need to take compression into account? Does it > > > affect the 20 MB limit? > > > > > > Regards, > > > Sushant Mahajan > > > > > > On Sun, 14 Jun 2026, 18:50 Muralidhar Basani via dev, < > > > [email protected]> > > > wrote: > > > > > > > Hi, > > > > Apologies for the delayed reply. > > > > > > > > Sushant, thanks for the reply. Just to flag the KIP has switched to > > > > bytes-based control (pls see below), which supersedes most of the > > > > records-based tuning discussion. > > > > > > > > Chia, thank you for the suggestions. Both the points are now in the > > KIP: > > > > - chia_00: broker floor stays at 0 (no compatibility break) > > > > - chia_01: bytes-based control adopted, matching > > > > metadata.log.max.record.bytes.between.snapshots, default 20 MB. > > > > > > > > Records-based config will be marked as deprecated. > > > > > > > > Updated KIP: > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1349%3A+Bytes-based+configurable+snapshot+frequency+for+share+groups > > > > > > > > Please take a look again. > > > > > > > > Thanks, > > > > Murali > > > > > > > > On Thu, Jun 11, 2026 at 5:20 AM Chia-Ping Tsai <[email protected]> > > > > wrote: > > > > > > > > > hi > > > > > > > > > > chia_00: Changing the lower bound of the existing config is a > > > > > compatibility issue. If we want to avoid heavy snapshots, we can > set > > > the > > > > > lower bound of the new group-level configuration to 200 by default. > > > Also, > > > > > the broker-level constraint should be changed to 0-1000, which > won’t > > > > bring > > > > > compatibility issues > > > > > > > > > > chia_01: Have you considered using bytes instead of records as the > > > > > controlled unit? > > > > > > > > > > > > > > > On 2026/05/22 19:30:38 Muralidhar Basani via dev wrote: > > > > > > Hi all, > > > > > > > > > > > > I would like to start a discussion on KIP-1349, which allows > > > > configurable > > > > > > snapshot frequency of share groups. > > > > > > > > > > > > KIP : > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1349%3A+Configurable+snapshot+frequency+for+share+groups > > > > > > > > > > > > Thanks, > > > > > > Murali > > > > > > > > > > > > > > > > > > > > >
