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