Thanks for making the changes, Tejal. The KIP looks great. Randall
On Wed, Apr 17, 2019 at 10:32 AM TEJAL ADSUL <te...@confluent.io> wrote: > > Thanks for the feedback Randall. I have incorporated the below changes in > the KIP. > > Thanks, > Tejal > > On 2019/04/17 15:42:31, Randall Hauch <rha...@gmail.com> wrote: > > Overall this looks good. A few specific requests to hopefully improve the > > clarity of the proposal: > > > > 1) The motivation section should more clearly state that this feature > will > > allow it to be reused in multiple components, Connect will use this > feature > > to resolve variables in the worker configurations via the existing > > 'StandaloneConfig’ and ‘DistributedConfig’ subclasses of AbstractConfig, > > that Connect already supports variables in connector configs via KIP-297, > > and that other components will have to be modified in the future to use > > this feature. Essentially, this builds on KIP-297 for variables in > Connect > > workers, but it does it in a way that other components can use in the > > future to automatically resolve variables in their configs with few > changes > > in code. > > > > 2) The Public Interfaces section talks about “the constructor”, but there > > are two new constructors presented, and each should be described. It > should > > also be more clear that the existing constructors will not enable > > resolution by default. > > > > 3) In several places were the new reserved configs are described, this > text > > appears: > > > > “and "config.providers.providerName.variableName" to specify any > > additional configs required by providers.“ > > > > This needs to make it more clear that “providerName” and “variableName” > are > > not literals; maybe surround them with angle brackets and call this out > > explicitly. > > > > 4) The “Dynamic configurations for Brokers” makes it sound like the > > broker’s behavior will change, but based on the discussions above it > sounds > > like only Connect will enable this feature. Is it even necessary to > mention > > this here? > > > > 5) Include as a rejected alternative the enabling by default that > existing > > AbstractConfig constructors will automatically resolve variables using > > config providers defined in the same configs. Instead, automatic > resolution > > must be explicitly enabled/used by the subclass for all components except > > Connect’s worker configurations. > > > > Best regards, > > > > Randall > > > > On Wed, Apr 17, 2019 at 7:49 AM TEJAL ADSUL <te...@confluent.io> wrote: > > > > > > > > Hi Colin, > > > > > > By default we are enabling this feature only Connect. All the other > > > components can enable or disable the feature as needed. > > > > > > No it won't reload the configuration if its not configured using KIP > 226 > > > as in order for the dynamic configs to be reloaded it has to be > triggered > > > by the user using the Admin Client. For static configs we dont perform > any > > > reloading and happens only at the construction time. > > > > > > Thanks, > > > Tejal > > > > > > On 2019/04/17 14:36:33, TEJAL ADSUL <te...@confluent.io> wrote: > > > > Hi Rajini, > > > > > > > > The user wont have the ability to choose whether config value should > be > > > enabled/disabled. Its enable/disabled per component by hardcoding the > > > value. I have documented your recommendations in the compatibility > sections. > > > > > > > > Thanks, > > > > Tejal > > > > > > > > > >
