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 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" ("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:
[email protected]
