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 > > > > > >
