ctubbsii commented on code in PR #3349:
URL: https://github.com/apache/accumulo/pull/3349#discussion_r1183213029
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -149,8 +149,8 @@ private TabletMetadataLoader(Ample ample) {
loadAll(Set<? extends KeyExtent> keys) {
long t1 = System.currentTimeMillis();
@SuppressWarnings("unchecked")
- var tms = ample.readTablets().forTablets((Collection<KeyExtent>)
keys).build().stream()
- .collect(Collectors.toMap(tm -> tm.getExtent(), tm -> tm));
+ var tms = ample.readTablets().forTablets((Collection<KeyExtent>) keys, e
-> {}).build()
Review Comment:
I still think the named function is easier to understand, and doesn't
require the unnecessary wrapping/unwrapping of a Consumer into an Optional.
While not important for performance reasons, I just don't think the boilerplate
wrapping/unwrapping is necessary or very clean. There is plenty of precedent
for naming functions for reuse like this, either by assigning to a variable or
by using a method reference. I just think it's cleaner, and makes the code much
easier to read. It's also [a pretty standard
recommendation](https://stackoverflow.com/a/29851786). But it's not that
important to me (not enough to put in the effort to persuade, anyway), and I
guess the Optional does achieve a similar purpose.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
