Hi all, If there are no further concerns, I'd like to start a vote soon.
Please let me know if any questions. Thanks, Murali On Sat, Jul 25, 2026 at 8:10 PM Muralidhar Basani < [email protected]> wrote: > Thanks Andrew, > I have updated kip - added the performance info to the motivation, > renamed the configs to the max.record.bytes.between.snapshots form, and > changed the per-group config importance level to LOW as you suggested. > > Pls let me know if you have any further feedback. > > Thanks, > Murali > > > On Fri, Jul 24, 2026 at 4:24 PM Andrew Schofield <[email protected]> > wrote: > >> Hi Murali, >> Thanks for your response. >> >> AS1: Thanks for the performance information. Adding this into the KIP is >> definitely helpful in the motivation. >> >> AS2: Thanks. Sounds good to me. >> >> AS3: HIGH and MEDIUM is fine with me, but my suggestion for LOW was >> because I think it will be rare for people to adjust the per-group config. >> It's a small point and happy for you to choose. >> >> Thanks, >> Andrew >> >> On 2026/07/16 21:12:58 Muralidhar Basani via dev wrote: >> > 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 >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > >> >
