Hi Randall, Looks great! Definitely in favor of a WARN level message instead of failing on startup. +1 non-binding when the vote thread comes
Cheers, Chris On Mon, May 4, 2020 at 12:51 PM Randall Hauch <rha...@gmail.com> wrote: > Thanks, Chris. > > 1. Added a use case that describes why you might want to use -1 for > replication factor but want to set other properties. > 2&3. Thanks for bringing up these special properties that the worker always > sets. I've added an "Excluded properties" column to the table of new > properties, and listed out the topic-specific properties should not be set. > I've also added this paragraph: > > Note that some topic-specific properties are excluded because the > distributed worker always sets specific values. Therefore, if a distributed > worker configuration does set any of these excluded properties, the > distributed worker will issue a warning that such properties should not be > set and will be ignored. > > > I'm proposing that using such properties will result in a WARN log message, > but allow the worker to continue by ignoring these values. I added a > rejected alternative describing why I think failing is not desirable. > > 4. Added a sentence as suggested. > > Thanks! > > On Sun, May 3, 2020 at 12:55 PM Christopher Egerton <chr...@confluent.io> > wrote: > > > Hi Randall, > > > > Thanks for the KIP! I have a few questions and suggestions but no major > > objections. > > > > 1. The motivation is pretty clear for altering the various > > "*.storage.replication.factor" properties to allow -1 as a value now. Are > > there expected use cases for allowing modification of other properties of > > these topic configs? It'd be nice to understand why we're adding this > extra > > configurability to the worker. > > > > 2. Should the "cleanup.policy" property have some additional guarding > logic > > to make sure that people don't set it to "delete" or "both"? > > > > 3. The lack of a "config.storage.partitions" property seems intentional > > because the config topic should only ever have one partition. Now that > > we're adding all of these other internal topic-related properties, do you > > think it might be helpful to users if we emit a warning message of some > > sort when they try to configure their worker with this property? > > > > 4. On the topic of compatibility--this is a fairly niche edge case, but > any > > time we add new configs to the worker we run the risk of overlap with > > existing configs for REST extensions that users may have implemented. > This > > is different from other pluggable interfaces like config providers and > > converters, whose properties are namespaced (presumably to avoid > collisions > > like this). Might be worth it to note this in a small paragraph or even > > just a single sentence. > > > > Cheers, > > > > Chris > > > > On Thu, Apr 30, 2020 at 4:32 PM Ryanne Dolan <ryannedo...@gmail.com> > > wrote: > > > > > Much needed, thanks. > > > > > > Ryanne > > > > > > On Thu, Apr 30, 2020 at 4:59 PM Randall Hauch <rha...@gmail.com> > wrote: > > > > > > > Hello! > > > > > > > > I'd like to use this thread to discuss KIP-605, which expands some of > > the > > > > properties that the Connect distributed worker uses when creating > > > internal > > > > topics: > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-605%3A+Expand+Connect+Worker+Internal+Topic+Settings > > > > > > > > Best regards, > > > > > > > > Randall > > > > > > > > > >
