Avoid unnecessary copies of Thrift binary types

.github/workflows/maven.yaml

@@ -54,6 +54,8 @@ jobs:
       run: src/build/ci/find-unapproved-abstract-ITs.sh
     - name: Check for unapproved package naming conventions
       run: src/build/ci/check-module-package-conventions.sh
+    - name: Check for unapproved bufferFor methods
+      run: src/build/ci/find-unapproved-thrift-bytebuffer.sh
     - name: Build with Maven (Fast Build)
       timeout-minutes: 20
       run: mvn -B -V -e -ntp "-Dstyle.color=always" clean package dependency:resolve -DskipTests -DskipFormat -DverifyFormat

core/src/main/java/org/apache/accumulo/core/data/Column.java

@@ -19,7 +19,6 @@
 package org.apache.accumulo.core.data;
 
 import static java.nio.charset.StandardCharsets.UTF_8;
-import static org.apache.accumulo.core.util.ByteBufferUtil.toBytes;
 
 import java.io.DataInput;
 import java.io.DataOutput;
@@ -139,8 +138,7 @@ public Column(byte[] columnFamily, byte[] columnQualifier, byte[] columnVisibili
    * @param tcol Thrift column
    */
   public Column(TColumn tcol) {
-    this(toBytes(tcol.bufferForColumnFamily()), toBytes(tcol.bufferForColumnQualifier()),
-        toBytes(tcol.bufferForColumnVisibility()));
+    this(tcol.getColumnFamily(), tcol.getColumnQualifier(), tcol.getColumnVisibility());
   }
 
   @Override

core/src/main/java/org/apache/accumulo/core/data/Key.java

@@ -29,7 +29,6 @@
 import org.apache.accumulo.core.dataImpl.thrift.TKey;
 import org.apache.accumulo.core.dataImpl.thrift.TKeyValue;
 import org.apache.accumulo.core.security.ColumnVisibility;
-import org.apache.accumulo.core.util.ByteBufferUtil;
 import org.apache.hadoop.io.Text;
 import org.apache.hadoop.io.WritableComparable;
 import org.apache.hadoop.io.WritableComparator;
@@ -685,10 +684,10 @@ public Key(Key other) {
    * @param tkey Thrift key
    */
   public Key(TKey tkey) {
-    this.row = ByteBufferUtil.toBytes(tkey.bufferForRow());
-    this.colFamily = ByteBufferUtil.toBytes(tkey.bufferForColFamily());
-    this.colQualifier = ByteBufferUtil.toBytes(tkey.bufferForColQualifier());
-    this.colVisibility = ByteBufferUtil.toBytes(tkey.bufferForColVisibility());
+    this.row = tkey.getRow();
+    this.colFamily = tkey.getColFamily();
+    this.colQualifier = tkey.getColQualifier();
+    this.colVisibility = tkey.getColVisibility();
     this.timestamp = tkey.getTimestamp();
     this.deleted = false;
 

core/src/main/java/org/apache/accumulo/core/data/Mutation.java

@@ -236,8 +236,8 @@ public Mutation() {}
    * @param tmutation Thrift mutation
    */
   public Mutation(TMutation tmutation) {
-    this.row = ByteBufferUtil.toBytes(tmutation.bufferForRow());
-    this.data = ByteBufferUtil.toBytes(tmutation.bufferForData());
+    this.row = tmutation.getRow();
+    this.data = tmutation.getData();
     this.entries = tmutation.getEntries();
     this.values = ByteBufferUtil.toBytesList(tmutation.getValues());
 

core/src/main/java/org/apache/accumulo/core/summary/Gatherer.java

@@ -71,7 +71,6 @@
 import org.apache.accumulo.core.spi.crypto.CryptoService;
 import org.apache.accumulo.core.tabletserver.thrift.TabletServerClientService.Client;
 import org.apache.accumulo.core.trace.TraceUtil;
-import org.apache.accumulo.core.util.ByteBufferUtil;
 import org.apache.accumulo.core.util.CancelFlagFuture;
 import org.apache.accumulo.core.util.CompletableFutureUtil;
 import org.apache.accumulo.core.util.TextUtil;
@@ -127,8 +126,8 @@ public Gatherer(ClientContext context, TSummaryRequest request, AccumuloConfigur
       CryptoService cryptoService) {
     this.ctx = context;
     this.tableId = TableId.of(request.getTableId());
-    this.startRow = ByteBufferUtil.toText(request.getBounds().bufferForStartRow());
-    this.endRow = ByteBufferUtil.toText(request.getBounds().bufferForEndRow());
+    this.startRow = TextUtil.fromNullableBytes(request.getBounds().getStartRow());
+    this.endRow = TextUtil.fromNullableBytes(request.getBounds().getEndRow());
     this.clipRange = new Range(startRow, false, endRow, true);
     this.summaries = request.getSummarizers().stream().map(SummarizerConfigurationUtil::fromThrift)
         .collect(Collectors.toSet());
@@ -436,8 +435,8 @@ public Future<SummaryCollection> processFiles(FileSystemResolver volMgr,
       Map<String,List<TRowRange>> files, BlockCache summaryCache, BlockCache indexCache,
       Cache<String,Long> fileLenCache, ExecutorService srp) {
     Function<TRowRange,RowRange> fromThrift = tRowRange -> {
-      Text lowerBound = ByteBufferUtil.toText(tRowRange.bufferForStartRow());
-      Text upperBound = ByteBufferUtil.toText(tRowRange.bufferForEndRow());
+      Text lowerBound = TextUtil.fromNullableBytes(tRowRange.getStartRow());
+      Text upperBound = TextUtil.fromNullableBytes(tRowRange.getEndRow());
       return RowRange.range(lowerBound, false, upperBound, true);
     };
     List<CompletableFuture<SummaryCollection>> futures = new ArrayList<>();

core/src/main/java/org/apache/accumulo/core/util/TextUtil.java

@@ -26,21 +26,18 @@
 import org.apache.hadoop.io.Text;
 
 public final class TextUtil {
+
+  public static Text fromNullableBytes(byte[] bytes) {
+    return bytes == null ? null : new Text(bytes);
+  }
+
   public static byte[] getBytes(Text text) {
     byte[] bytes = text.getBytes();
-    if (bytes.length != text.getLength()) {
-      bytes = new byte[text.getLength()];
-      System.arraycopy(text.getBytes(), 0, bytes, 0, bytes.length);
-    }
-    return bytes;
+    return bytes.length == text.getLength() ? bytes : text.copyBytes();
   }
 
   public static ByteBuffer getByteBuffer(Text text) {
-    if (text == null) {
-      return null;
-    }
-    byte[] bytes = text.getBytes();
-    return ByteBuffer.wrap(bytes, 0, text.getLength());
+    return text == null ? null : ByteBuffer.wrap(text.getBytes(), 0, text.getLength());
   }
 
   public static Text truncate(Text text, int maxLen) {
@@ -51,7 +48,6 @@ public static Text truncate(Text text, int maxLen) {
       newText.append(suffix.getBytes(UTF_8), 0, suffix.length());
       return newText;
     }
-
     return text;
   }
 

core/src/main/scripts/generate-thrift.sh

@@ -67,7 +67,7 @@ THRIFT_ARGS=("${THRIFT_ARGS[@]}" -o "$BUILD_DIR")
 mkdir -p "$BUILD_DIR"
 rm -rf "$BUILD_DIR"/gen-java
 for f in src/main/thrift/*.thrift; do
-  thrift "${THRIFT_ARGS[@]}" --gen java:generated_annotations=suppress,private-members "$f" || fail unable to generate java thrift classes
+  thrift "${THRIFT_ARGS[@]}" --gen java:generated_annotations=suppress,private_members "$f" || fail unable to generate java thrift classes
   thrift "${THRIFT_ARGS[@]}" --gen py "$f" || fail unable to generate python thrift classes
   thrift "${THRIFT_ARGS[@]}" --gen rb "$f" || fail unable to generate ruby thrift classes
   thrift "${THRIFT_ARGS[@]}" --gen cpp "$f" || fail unable to generate cpp thrift classes
@@ -76,10 +76,12 @@ done
 # For all generated thrift code, get rid of all warnings and add the LICENSE header
 
 # add dummy method to suppress "unnecessary suppress warnings" for classes which don't have any unused variables
-# this only affects classes, enums aren't affected
+# this only affects classes, enums aren't affected; also avoid thrift bug that makes a redundant copy when resizing the array
 #shellcheck disable=SC1004
-find "$BUILD_DIR/gen-java" -name '*.java' -exec grep -Zl '^public class ' {} + | xargs -0 sed -i -e 's/^[}]$/  private static void unusedMethod() {}\
-}/'
+find "$BUILD_DIR/gen-java" -name '*.java' -exec grep -Zl '^public class ' {} + | xargs -0 sed -i \
+  -e 's/^[}]$/  private static void unusedMethod() {}\
+}/' \
+  -e 's/^\([[:space:]]*\)set[A-Z][A-Za-z]*(org[.]apache[.]thrift[.]TBaseHelper[.]rightSize(\([^)]*\)));$/\1this.\2 = org.apache.thrift.TBaseHelper.rightSize(\2);/g'
 
 for lang in "${LANGUAGES_TO_GENERATE[@]}"; do
   case $lang in