[GitHub] [kafka] VJvaLbhYbfr edited a comment on pull request #10792: MINOR: Small refactor of tests

[GitBox <gi...@apache.org>]

VJvaLbhYbfr edited a comment on pull request #10792:
URL: https://github.com/apache/kafka/pull/10792#issuecomment-850922135


   > 
   > 
   > 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 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 started to being used in next few days. "Coding for the future" (which 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:
users@infra.apache.org