VJvaLbhYbfr edited a comment on pull request #10792: URL: https://github.com/apache/kafka/pull/10792#issuecomment-850922135
Thank you for your review. > > > In fact, `Collections.singleton` is more efficient than other collection constructor and the `highWatermark` is somehow prepared for future test, but I think this PR is feasible. That is correct, `Collection.singleton` is more efficient than, for example, `Utils.mkSet` for a single element, but I made this decision mainly because of two reasons: 1. IMO consistency is more important than minor performance gain, especially in a case of tests or a code which is not a "hot spot", 2. this is kinda an implementation detail that one version is faster than the other. Implementation can change -`Utils.mkSet` might get an overloaded version with a single argument and not return `HashSet`, but `SingletonSet`. Mainly reason nr 1. Reason nr 2 is more design-philosophical. Probably the best solution would be to use `java.util.Set.of(...)`. It would provide both consistency and performance. And it's from JDK, so it wouldn't require maintenance from our side as `Utils.mkSet(...)` does. Should I refactor to `java.util.Set.of(...)`? About `highWatermark` - I wasn't aware that something is coming in the future. That's your call then. If it would be up to me, then I would probably do this change, unless `highWatermark` parameter would be being used in next few days. "Coding for the future" (and "future" is usually very variable/volatile in time) is always risky, because it impacts readibility today without gaining any advantage today. And pushing `Optional.empty()` to method signature is one hotkey in IDEA ;) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
