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