ctubbsii commented on code in PR #3501:
URL: https://github.com/apache/accumulo/pull/3501#discussion_r1231511531
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -70,13 +70,16 @@
import
org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection.TabletColumnFamily;
import org.apache.accumulo.core.tabletserver.log.LogEntry;
import org.apache.accumulo.core.util.HostAndPort;
+import org.apache.accumulo.core.util.Pair;
import org.apache.accumulo.core.util.ServerServices;
import org.apache.hadoop.io.Text;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
+import com.google.common.base.Supplier;
Review Comment:
This is almost certainly going to trigger modernizer with a suggestion to
use `java.util.function.Supplier`. It is possible to use Guava `Suppliers` with
the Java functional API, though. I think we have cases where we do that, so it
should be easy to change this.
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -506,6 +518,12 @@ public static <E extends Entry<Key,Value>> TabletMetadata
convertRow(Iterator<E>
if (buildKeyValueMap) {
te.keyValues = kvBuilder.build();
}
+ te.filesSupplier = Suppliers.memoize(() -> {
+ ImmutableMap.Builder<StoredTabletFile,DataFileValue> fb =
ImmutableMap.builder();
+ te.rawFiles.forEach(pair -> fb.put(new StoredTabletFile(pair.getFirst()),
+ new DataFileValue((pair.getSecond()))));
+ return fb.build();
Review Comment:
The Guava immutable types are nice APIs to work with, but for simple
immutable collections that are constructed in one small part of the code, it's
also pretty easy to use something like
`.stream()...collect(Collectors.toUnmodifiableMap())` to avoid more dependence
on Guava entirely. This might be easier if using a multimap for the source of
the stream.
Here's some other ideas:
https://www.baeldung.com/java-stream-immutable-collection
I just think using `.forEach(map.put)` is probably the less readable than
`stream.collect(mapcollector)`. It's direct, and simple enough, as a programmer
to understand.
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -91,7 +94,8 @@ public class TabletMetadata {
private boolean sawOldPrevEndRow = false;
private Text endRow;
private Location location;
- private Map<StoredTabletFile,DataFileValue> files;
+ private List<Pair<String,String>> rawFiles;
Review Comment:
Would a multimap make more sense here?
##########
core/src/main/java/org/apache/accumulo/core/util/Pair.java:
##########
@@ -23,8 +23,8 @@
import java.util.Objects;
public class Pair<A,B> {
- A first;
- B second;
+ final A first;
+ final B second;
Review Comment:
Do you know why these are visible? They could probably be private, too.
--
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]
