Thanks Sushant & Andrew. AS1 On performance testing so far:
I ran a test on a local 4.3 cluster to see how big the snapshot related records actually are. +++++++++++ config_value,record_count,snapshot_bytes,update_bytes,snapshot_count,update_count,snapshot_byte_ratio,avg_update_bytes,avg_snapshot_bytes,updates_per_20mb,cliff_ratio_vs_N,broker_restart_load_secs 50,200000,1284,51561,8,337,0.024,153.0,160.5,137069,2741.4,3 100,200000,592,52206,4,339,0.011,154.0,148.0,136179,1361.8,3 200,200000,592,52360,4,340,0.011,154.0,148.0,136179,680.9,4 300,200000,592,52514,4,341,0.011,154.0,148.0,136179,453.9,4 500,200000,592,52206,4,339,0.011,154.0,148.0,136179,272.4,4 +++++++++++ (Note: update_count is low because acks were in-order. pls see the caveat below.) The main finding: each "update" record is tiny, about 154 bytes on average. This stayed the same no matter what I changed, because these records only store small (bookkeeping) data (which messages were delivered/acked), not the message contents. Currently the old setting takes a snapshot every N update records (max 500). The new setting takes a snapshot every 20 MB. Since each update is only ~154 bytes, 20 MB works out to about 136,000 updates between snapshots. So when we eventually remove the old config, a group that used to snapshot every 500 updates would instead snapshot every ~136,000, roughly ~270× less often at the old maximum of 500, and even more dramatic at lower settings (up to ~2700× at N=50). Fewer snapshots means recovery after a restart takes longer and more log is kept on disk before cleanup, because there are more update records to replay before reaching a snapshot. It does not affect throughput. In my test, the consumer acknowledged messages in order, so 200,000 messages collapsed into only ~340 update records. That was enough to measure the size of an update, but it did not trigger the snapshot limits, so restart time stayed flat (~3–4s) in every run. To properly measure the recovery-time difference, I'll use a consumer that acknowledges out of order, which produces many more (and larger) update records and actually exercises the snapshot limits, then compare restart time and on-disk log size at the old vs. new settings. Agree, the new code does a little extra work per update (one more counter, plus measuring each record's size). I'll benchmark that against trunk during impl, before merge. I will add a performance investigation note to the KIP's Test Plan to capture this. AS2 Good point. The name I proposed (share.coordinator.snapshot.update.bytes.per.snapshot) was derived from the existing config and swapped "records" for "bytes". But this bytes-based design is modeled on metadata.log.max.record.bytes.between.snapshots, so happy to rename. I can rename both configs: - broker: share.coordinator.snapshot.update.bytes.per.snapshot to share.coordinator.max.record.bytes.between.snapshots - per-group: share.snapshot.update.bytes.per.snapshot to share.max.record.bytes.between.snapshots AS3 Broker-level HIGH: agree Group-level LOW: I see the reasoning, but every existing per-group config in GroupConfig currently is MEDIUM. (including the other share.* group configs like share.record.lock.duration.ms). So LOW would make this the only LOW group config. Slight lean toward MEDIUM for consistency, but no strong objection to LOW. Pls let me know your thoughts. Thanks, Murali On Thu, Jul 16, 2026 at 3:55 PM Andrew Schofield <[email protected]> wrote: > Hi Murali, > Thanks for the KIP. I understand the proposal. > > AS1: What performance testing have you done to validate whether there is a > performance impact due to this change? When we get to the point of removing > the deprecated configuration, there is a potential behavioural cliff which > we should have investigated before we get there. > > AS2: Is there any reason why you did not follow a similar naming scheme as > the existing metadata log configs, such as > share.coordinator.max.record.bytes.between.snapshots? > > AS3: I suggest that the broker-level config would be HIGH and the > group-level config would be LOW importance. > > Thanks, > Andrew > > On 2026/07/16 05:13:29 Sushant Mahajan wrote: > > Thanks for the change. > > > > All comments from my side are addressed. > > > > Regards, > > Sushant Mahajan > > > > On Wed, 15 Jul 2026, 00:37 Muralidhar Basani via dev, < > [email protected]> > > wrote: > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
