From 384614590ffaaafec259e0d2c3712311bd4fe4df Mon Sep 17 00:00:00 2001 From: David Mollitor Date: Fri, 23 Aug 2019 16:06:40 -0400 Subject: [PATCH 1/2] Prefer ArrayList over LinkedList and native array --- .../accumulo/core/file/rfile/RFile.java | 73 +++++++++---------- .../system/LocalityGroupIterator.java | 9 ++- 2 files changed, 42 insertions(+), 40 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java b/core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java index cdbfd4f6b08..d03ba56f862 100644 --- a/core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java +++ b/core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java @@ -30,7 +30,6 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; -import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -1132,12 +1131,12 @@ public static class Reader extends HeapIterator implements FileSKVIterator { private final CachableBlockFile.Reader reader; - private final ArrayList localityGroups = new ArrayList<>(); - private final ArrayList sampleGroups = new ArrayList<>(); + private final List localityGroups = new ArrayList<>(); + private final List sampleGroups = new ArrayList<>(); - private final LocalityGroupReader[] currentReaders; - private final LocalityGroupReader[] readers; - private final LocalityGroupReader[] sampleReaders; + private final List currentReaders; + private final List readers; + private final List sampleReaders; private final LocalityGroupContext lgContext; private LocalityGroupSeekCache lgCache; @@ -1166,29 +1165,27 @@ public Reader(CachableBlockFile.Reader rdr) throws IOException { throw new IOException("Did not see expected version, saw " + ver); int size = mb.readInt(); - currentReaders = new LocalityGroupReader[size]; + currentReaders = new ArrayList<>(size); - deepCopies = new LinkedList<>(); + deepCopies = new ArrayList<>(); for (int i = 0; i < size; i++) { LocalityGroupMetadata lgm = new LocalityGroupMetadata(ver, rdr); lgm.readFields(mb); localityGroups.add(lgm); - - currentReaders[i] = new LocalityGroupReader(reader, lgm, ver); + currentReaders.add(new LocalityGroupReader(reader, lgm, ver)); } readers = currentReaders; if (ver == RINDEX_VER_8 && mb.readBoolean()) { - sampleReaders = new LocalityGroupReader[size]; + sampleReaders = new ArrayList<>(size); for (int i = 0; i < size; i++) { LocalityGroupMetadata lgm = new LocalityGroupMetadata(ver, rdr); lgm.readFields(mb); sampleGroups.add(lgm); - - sampleReaders[i] = new LocalityGroupReader(reader, lgm, ver); + sampleReaders.add(new LocalityGroupReader(reader, lgm, ver)); } samplerConfig = new SamplerConfigurationImpl(mb); @@ -1203,30 +1200,30 @@ public Reader(CachableBlockFile.Reader rdr) throws IOException { lgContext = new LocalityGroupContext(currentReaders); - createHeap(currentReaders.length); + createHeap(currentReaders.size()); } - private Reader(Reader r, LocalityGroupReader[] sampleReaders) { - super(sampleReaders.length); + private Reader(Reader r, List sampleReaders) { + super(sampleReaders.size()); this.reader = r.reader; - this.currentReaders = new LocalityGroupReader[sampleReaders.length]; + this.currentReaders = new ArrayList<>(sampleReaders.size()); this.deepCopies = r.deepCopies; this.deepCopy = false; this.readers = r.readers; this.sampleReaders = r.sampleReaders; this.samplerConfig = r.samplerConfig; this.rfileVersion = r.rfileVersion; - for (int i = 0; i < sampleReaders.length; i++) { - this.currentReaders[i] = sampleReaders[i]; - this.currentReaders[i].setInterruptFlag(r.interruptFlag); + for (LocalityGroupReader reader : sampleReaders) { + reader.setInterruptFlag(r.interruptFlag); + this.currentReaders.add(reader); } this.lgContext = new LocalityGroupContext(currentReaders); } private Reader(Reader r, boolean useSample) { - super(r.currentReaders.length); + super(r.currentReaders.size()); this.reader = r.reader; - this.currentReaders = new LocalityGroupReader[r.currentReaders.length]; + this.currentReaders = new ArrayList<>(r.currentReaders.size()); this.deepCopies = r.deepCopies; this.deepCopy = true; this.samplerConfig = r.samplerConfig; @@ -1234,15 +1231,11 @@ private Reader(Reader r, boolean useSample) { this.readers = r.readers; this.sampleReaders = r.sampleReaders; - for (int i = 0; i < r.readers.length; i++) { - if (useSample) { - this.currentReaders[i] = new LocalityGroupReader(r.sampleReaders[i]); - this.currentReaders[i].setInterruptFlag(r.interruptFlag); - } else { - this.currentReaders[i] = new LocalityGroupReader(r.readers[i]); - this.currentReaders[i].setInterruptFlag(r.interruptFlag); - } - + List readers = (useSample) ? r.sampleReaders : r.readers; + for (LocalityGroupReader reader : readers) { + LocalityGroupReader newReader = new LocalityGroupReader(reader); + newReader.setInterruptFlag(r.interruptFlag); + this.currentReaders.add(newReader); } this.lgContext = new LocalityGroupContext(currentReaders); } @@ -1263,11 +1256,13 @@ private void closeLocalityGroupReaders() { @Override public void closeDeepCopies() { - if (deepCopy) + if (deepCopy) { throw new RuntimeException("Calling closeDeepCopies on a deep copy is not supported"); + } - for (Reader deepCopy : deepCopies) + for (Reader deepCopy : deepCopies) { deepCopy.closeLocalityGroupReaders(); + } deepCopies.clear(); } @@ -1301,7 +1296,7 @@ public void close() throws IOException { @Override public Key getFirstKey() throws IOException { - if (currentReaders.length == 0) { + if (currentReaders.isEmpty()) { return null; } @@ -1322,7 +1317,7 @@ public Key getFirstKey() throws IOException { @Override public Key getLastKey() throws IOException { - if (currentReaders.length == 0) { + if (currentReaders.isEmpty()) { return null; } @@ -1398,7 +1393,7 @@ public Map> getLocalityGroupCF() { if (lcg.columnFamilies == null) { Preconditions.checkState(lcg.isDefaultLG, "Group %s has null families. " + "Only expect default locality group to have null families.", lcg.name); - setCF = new ArrayList<>(); + setCF = new ArrayList<>(0); } else { setCF = new ArrayList<>(lcg.columnFamilies.keySet()); } @@ -1497,11 +1492,13 @@ public void printInfo(boolean includeIndexDetails) throws IOException { @Override public void setInterruptFlag(AtomicBoolean flag) { - if (deepCopy) + if (deepCopy) { throw new RuntimeException("Calling setInterruptFlag on a deep copy is not supported"); + } - if (deepCopies.size() != 0) + if (!deepCopies.isEmpty()) { throw new RuntimeException("Setting interrupt flag after calling deep copy not supported"); + } setInterruptFlagInternal(flag); } diff --git a/core/src/main/java/org/apache/accumulo/core/iterators/system/LocalityGroupIterator.java b/core/src/main/java/org/apache/accumulo/core/iterators/system/LocalityGroupIterator.java index c6f9581b7f9..35c41abe972 100644 --- a/core/src/main/java/org/apache/accumulo/core/iterators/system/LocalityGroupIterator.java +++ b/core/src/main/java/org/apache/accumulo/core/iterators/system/LocalityGroupIterator.java @@ -72,8 +72,8 @@ public static class LocalityGroupContext { final LocalityGroup defaultGroup; final Map groupByCf; - public LocalityGroupContext(LocalityGroup[] groups) { - this.groups = Collections.unmodifiableList(Arrays.asList(groups)); + public LocalityGroupContext(final List groups) { + this.groups = Collections.unmodifiableList(groups); this.groupByCf = new HashMap<>(); LocalityGroup foundDefault = null; @@ -96,6 +96,11 @@ public LocalityGroupContext(LocalityGroup[] groups) { } defaultGroup = foundDefault; } + + @Deprecated + public LocalityGroupContext(LocalityGroup[] groups) { + this(Arrays.asList(groups)); + } } /** From 1c01ca196346f5c7b8c7a9a76a15dad58063f40e Mon Sep 17 00:00:00 2001 From: David Mollitor Date: Tue, 10 Sep 2019 21:28:47 -0400 Subject: [PATCH 2/2] Made changes based on code review --- .../java/org/apache/accumulo/core/file/rfile/RFile.java | 3 ++- .../core/iterators/system/LocalityGroupIterator.java | 7 +------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java b/core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java index d03ba56f862..12bf18d7516 100644 --- a/core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java +++ b/core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java @@ -30,6 +30,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; +import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -1167,7 +1168,7 @@ public Reader(CachableBlockFile.Reader rdr) throws IOException { int size = mb.readInt(); currentReaders = new ArrayList<>(size); - deepCopies = new ArrayList<>(); + deepCopies = new LinkedList<>(); for (int i = 0; i < size; i++) { LocalityGroupMetadata lgm = new LocalityGroupMetadata(ver, rdr); diff --git a/core/src/main/java/org/apache/accumulo/core/iterators/system/LocalityGroupIterator.java b/core/src/main/java/org/apache/accumulo/core/iterators/system/LocalityGroupIterator.java index 35c41abe972..41ce3d62bbb 100644 --- a/core/src/main/java/org/apache/accumulo/core/iterators/system/LocalityGroupIterator.java +++ b/core/src/main/java/org/apache/accumulo/core/iterators/system/LocalityGroupIterator.java @@ -96,11 +96,6 @@ public LocalityGroupContext(final List groups) { } defaultGroup = foundDefault; } - - @Deprecated - public LocalityGroupContext(LocalityGroup[] groups) { - this(Arrays.asList(groups)); - } } /** @@ -134,7 +129,7 @@ public int getNumLGSeeked() { public LocalityGroupIterator(LocalityGroup[] groups) { super(groups.length); - this.lgContext = new LocalityGroupContext(groups); + this.lgContext = new LocalityGroupContext(Arrays.asList(groups)); } @Override