Hey Hudeqi, Thanks for the KIP! After reading the KIP and the comments by Yash and Greg I agree with these aspects:
1) While I agree that having a high value for segment.btes config can lead to higher startup times, we don't necessarily need to expose a separate config for it(as Yash suggested). We can use the same mechanism as exposed by KIP-605 to set it to a lower value specifically for Connect. IMO this could be an internal config as well defined via ConfigDef#defineInternal so they don't need to show up in the docs as well. I haven't tested it but if the users do happen to override the config via the KIP-605 mechanism, it should update. So, the scope of the KIP could be reduced to having an explicit internal config for offset's topic segment.bytes with a lower default value. WDYT? 2) I don't think we should let the configs of existing topics be updated. If you notice both KIP-605 and KIP-154 (the one which 605 cites) don't allow updating the configs of existing topics. It would be a good idea to stick around with this practice imo. 3) Regarding the default value of 50 MB, tbh I am not totally aware of how the default values for these configs were chosen in the past. But as pointed out by Greg, __consumer_offsets topic could be a good example to follow and a default value of 100MB could be a good starting point. Or if needed we can be defensive and start with a slightly higher value like 250MB. Also the point about tombstone records leading to inconsistent in-memory states across multiple workers is a good one. This happens with status topic as well IIRC and if possible we should look to fix it. That is outside the scope of the KIP though. Thanks! Sagar. On Fri, Jul 14, 2023 at 1:49 AM Greg Harris <greg.har...@aiven.io.invalid> wrote: > Hey hudeqi, > > Thanks for the KIP! I did not know about the existing segment.bytes > default value, and it does seem rather high in the context of the > Connect internal topics. > If I think about the segment.size as a "minimum per-partition data > transfer on startup", 1GB is certainly not appropriate for even the > single-partition config topic. > > 1. I have a concern about changing the topic configuration on startup. > > In the existing codebase, the *.storage.* worker configurations appear > to only have an effect for newly created topics. If the topics were > manually created before a Connect cluster starts, or were created by a > previous Connect instance, then the Connect worker configuration could > have arbitrary contents that have no effect. Updating the topic > configurations after creation would be a new capability. > Consider the situation where someone were to notice this log.segment > problem, where a natural response would be to reconfigure the topic, > diverging from the two configurations. When the worker can change the > topic configuration after creation, that has the potential to roll > back topic configurations that are managed externally. > Do you think that changing the default for new Connect clusters, and > emitting a startup warning for excessive segment.bytes is reasonable? > We have other startup assertions that fail the startup of a worker > based on partition and compaction requirements, and this would be > similar in that it alerts the user to reconfigure the internal topics, > but with a lesser severity. > > 2. I'm also interested to know what a reasonable value for this > configuration would be. I did find the __consumer_offsets topic uses > 104857600 (100 MiB) as defined in OffsetConfig.scala, so there is > precedent for having a smaller segment.size for internal topics. > > 3. I believe there's a potential bug where compaction can happen > before a worker reads a tombstone, leading the KafkaBasedLog to > produce inconsistent in-memory states across multiple workers. Since > the segment.size is so large, it makes me think that compaction has > been wholly ineffective so far, and has prevented this bug from > manifesting. By lowering the segment.size, we're increasing the > likelihood of this failure, so it may need to finally be addressed. > > Thanks, > Greg > > > > > On Thu, Jul 6, 2023 at 5:39 AM Yash Mayya <yash.ma...@gmail.com> wrote: > > > > Also, just adding to the above point - we don't necessarily need to > > explicitly add a new worker configuration right? Instead, we could > > potentially just use the new proposed default value internally which can > be > > overridden by users through setting a value for > > "offset.storage.segment.bytes" (via the existing KIP-605 based > mechanism). > > > > On Thu, Jul 6, 2023 at 6:04 PM Yash Mayya <yash.ma...@gmail.com> wrote: > > > > > Hi hudeqi, > > > > > > Thanks for the KIP! Just to clarify - since KIP-605 ( > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-605%3A+Expand+Connect+Worker+Internal+Topic+Settings > ) > > > already allows configuring "segment.bytes" for the Connect cluster's > > > offsets topic via a worker configuration > ("offset.storage.segment.bytes", > > > same as what is being proposed in this KIP), the primary motivation of > this > > > KIP is essentially to override the default value for that topic > > > configuration to a smaller value and decouple it from the backing Kafka > > > cluster's "log.segment.bytes" configuration? Also, I'm curious about > how > > > the new default value of 50 MB was chosen (if there were any > experiments > > > that were run etc.)? > > > > > > Thanks, > > > Yash > > > > > > On Mon, Jul 3, 2023 at 6:08 PM hudeqi <16120...@bjtu.edu.cn> wrote: > > > > > >> Is anyone following this KIP? Bump this thread. > > > > > > >
