Thanks for driving this KIP, Colin. I agree with Dong that a new similar modifyConfigs API (and protocol API) is confusing and that we should try to extend the current alterConfigs interface to support the incremental mode instead, deprecating the non-incremental mode in the process.
Another thing that I believe is missing is a per-config-entry operation type, namely SET (ovewrite), APPEND or DELETE. The current proposal only has SET (changes) and DELETE (removals) semantics, but I understand there are configuration properties (such as SASL auth) where it should be possible to APPEND to a configuration property, otherwise we'll have the same non-atomic read-modify-write cycle problems as with the current API. Instead of providing two sets of config: changes and removals, I think it might be better to just have one set where each Config entry has the operation type set, this also voids any confusion on what happens if a property is included in both changes,removals sets. Regards, Magnus 2018-07-16 20:23 GMT+02:00 Dong Lin <lindon...@gmail.com>: > Hey Colin, > > Thanks much for the explanation. Yeah it makes sense to deprecate the > existing non-incremental mode completely. LGTM. I just have two minor > comments. > > I am not too strong with the following argument but it may be useful to > just put it here for discussion. I am still wondering whether we can just > overload alterConfigs(...) instead of using modifyConfigs(...). In the > "Rejected Alternatives" section it is said that this approach does not > allow us to deprecate the non-incremental mode. Not sure why it is the > case. Can you explain a bit more? > > Regarding whether this approach is surprising and baffling to users, > personally I feel that with the existing approach in the KIP, a new name > such as modifyConfigs(...) does make it very explicit that this API is > different from the existing alterConfigs(...). But it does not really tell > user how they are different and users will have to read the Java doc to > figure this out. On the other hand, if we just overload the > alterConfigs(...) and deprecate the existing non-incremental > alterConfigs(...), it would also make it reasonable clear to user that the > two methods are different. Commonly-used IDE such as IntellIj would show > that one API is deprecated and the other is not. And user would then read > the Java doc to understand the difference. So the difference between these > two approaches in the short term is probably not much. And in the long term > it might be preferred to use "alter" instead of "modify". > > Another minor comment: should we include specify in the "Compatibility, > Deprecation, and Migration Plan" section that we intend to deprecate the > existing API? And do we plan to deprecate the > AlterConfigsRequest/AlterConfigsResponse > as well? The latter may be important to non-Kafka projects that have > implemented AdminClient interface. > > > Thanks, > Dong > > > On Mon, Jul 16, 2018 at 9:43 AM, Colin McCabe <cmcc...@apache.org> wrote: > > > On Fri, Jul 13, 2018, at 23:20, Dong Lin wrote: > > > Hey Colin, > > > > > > Thanks for the KIP! > > > > > > It seems that the AlterConfigsResult is pretty much the same as > > > ModifyConfigsResult. Instead of adding ModifyConfigs API and > deprecating > > > AlterConfigs API, would it be simpler to just add API alterConfigs( > > > Map<ConfigResource, Config> changes, Set<ConfigResource> removals, > final > > > ModifyConfigsOptions options)? > > > > > > Currently we use the word "alter" in method names such as > > > "alterReplicaLogDirs" and "alterCheckpointDir". So it is probably more > > > preferred to keep using the word "alter" instead of "modify" if > > posssible. > > > And if we can overload the alterConfigs(...) API to allow incremental > > > change, it might make sense to keep the existing > > > alterConfigs(Map<ConfigResource, > > > Config> configs) for users who simply want to overwrite the entire > > configs. > > > > If we have two functions with these type signatures: > > > > > AlterConfigsResult alterConfigs(Map<ConfigResource, Config> changes); > > > AlterConfigsResult alterConfigs(Map<ConfigResource, Config> changes, > > Set<ConfigResource> removals); > > > > It will be extremely surprising, even baffling, to users that the second > > function overload makes incremental changes, whereas the first function > > overload clears the entire configuration before applying changes. Just > > looking at the type signatures (which is all most developers will look > at, > > especially if they're using IDE autocomplete), you would not expect such > a > > radical difference between them. You would expect the second one to work > > just like the first, except maybe it would also perform some removals. > > > > Calling the two functions different names is good because it reflects the > > fact that they are very different. > > > > > And those user would not have to make code change due to API > deprecation. > > > What do you think? > > > > alterConfigs needs to be deprecated, though. Code using alterConfigs is > > almost certainly buggy because of the possibility of simultaneous > > read-modify-write cycles, and the fact that some configs can't be read. > > > > best, > > Colin > > > > > > > > Thanks, > > > Dong > > > > > > On Wed, Jul 11, 2018 at 10:54 AM, Colin McCabe <cmcc...@apache.org> > > wrote: > > > > > > > Hi all, > > > > > > > > Previously, we discussed some issues with alterConfigs here on the > > mailing > > > > list, and eventually came to the conclusion that the RPC as > implemented > > > > can't be used for a shell command modifying configurations. > > > > > > > > I wrote up a small KIP to fix the issues with the RP Please take a > > look > > > > at https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > > 339%3A+Create+a+new+ModifyConfigs+API > > > > > > > > best, > > > > Colin > > > > > > >
