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