From 6fb2f4e2314790b01b349c200809a537c6b093df Mon Sep 17 00:00:00 2001 From: Christopher Tubbs Date: Wed, 1 Jul 2026 19:26:33 -0400 Subject: [PATCH 1/4] Avoid unnecessary copies of Thrift binary types --- .../main/java/org/apache/accumulo/core/data/Column.java | 4 +--- .../src/main/java/org/apache/accumulo/core/data/Key.java | 9 ++++----- .../java/org/apache/accumulo/core/data/Mutation.java | 4 ++-- .../java/org/apache/accumulo/core/summary/Gatherer.java | 9 ++++----- core/src/main/scripts/generate-thrift.sh | 2 +- 5 files changed, 12 insertions(+), 16 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/data/Column.java b/core/src/main/java/org/apache/accumulo/core/data/Column.java index 7f6698e39cb..1817b999c3a 100644 --- a/core/src/main/java/org/apache/accumulo/core/data/Column.java +++ b/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 diff --git a/core/src/main/java/org/apache/accumulo/core/data/Key.java b/core/src/main/java/org/apache/accumulo/core/data/Key.java index f0b5e795660..fab52b37947 100644 --- a/core/src/main/java/org/apache/accumulo/core/data/Key.java +++ b/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; diff --git a/core/src/main/java/org/apache/accumulo/core/data/Mutation.java b/core/src/main/java/org/apache/accumulo/core/data/Mutation.java index 00e401218ea..68dbdeb1867 100644 --- a/core/src/main/java/org/apache/accumulo/core/data/Mutation.java +++ b/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()); diff --git a/core/src/main/java/org/apache/accumulo/core/summary/Gatherer.java b/core/src/main/java/org/apache/accumulo/core/summary/Gatherer.java index d26b80f7613..0ba937d8655 100644 --- a/core/src/main/java/org/apache/accumulo/core/summary/Gatherer.java +++ b/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 = new Text(request.getBounds().getStartRow()); + this.endRow = new Text(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 processFiles(FileSystemResolver volMgr, Map> files, BlockCache summaryCache, BlockCache indexCache, Cache fileLenCache, ExecutorService srp) { Function fromThrift = tRowRange -> { - Text lowerBound = ByteBufferUtil.toText(tRowRange.bufferForStartRow()); - Text upperBound = ByteBufferUtil.toText(tRowRange.bufferForEndRow()); + Text lowerBound = new Text(tRowRange.getStartRow()); + Text upperBound = new Text(tRowRange.getEndRow()); return RowRange.range(lowerBound, false, upperBound, true); }; List> futures = new ArrayList<>(); diff --git a/core/src/main/scripts/generate-thrift.sh b/core/src/main/scripts/generate-thrift.sh index f47b30ff957..d161b591a35 100755 --- a/core/src/main/scripts/generate-thrift.sh +++ b/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 From 1f53c0e605b0463e2ec238dd376591885d487961 Mon Sep 17 00:00:00 2001 From: Christopher Tubbs Date: Thu, 2 Jul 2026 20:37:01 -0400 Subject: [PATCH 2/4] Work around bug in thrift generated code Avoid making copies using the public API setter when truncating the underlying stream to get the array by assigning the result of the optional resize operation directly to the variable, and avoiding the setter that performs the unneeded copy. --- core/src/main/scripts/generate-thrift.sh | 8 +++++--- .../accumulo/core/clientImpl/thrift/ClientService.java | 4 ++-- .../apache/accumulo/core/dataImpl/thrift/TColumn.java | 6 +++--- .../accumulo/core/dataImpl/thrift/TCondition.java | 10 +++++----- .../org/apache/accumulo/core/dataImpl/thrift/TKey.java | 8 ++++---- .../accumulo/core/dataImpl/thrift/TKeyExtent.java | 6 +++--- .../accumulo/core/dataImpl/thrift/TKeyValue.java | 2 +- .../accumulo/core/dataImpl/thrift/TMutation.java | 4 ++-- .../accumulo/core/dataImpl/thrift/TRowRange.java | 4 ++-- .../core/manager/thrift/ManagerClientService.java | 4 ++-- .../core/securityImpl/thrift/TAuthenticationKey.java | 2 +- .../core/securityImpl/thrift/TCredentials.java | 2 +- .../core/securityImpl/thrift/TDelegationToken.java | 2 +- .../tabletserver/thrift/TabletServerClientService.java | 4 ++-- 14 files changed, 34 insertions(+), 32 deletions(-) diff --git a/core/src/main/scripts/generate-thrift.sh b/core/src/main/scripts/generate-thrift.sh index d161b591a35..1e03442f168 100755 --- a/core/src/main/scripts/generate-thrift.sh +++ b/core/src/main/scripts/generate-thrift.sh @@ -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 diff --git a/core/src/main/thrift-gen-java/org/apache/accumulo/core/clientImpl/thrift/ClientService.java b/core/src/main/thrift-gen-java/org/apache/accumulo/core/clientImpl/thrift/ClientService.java index 83ede224667..acaf3649eab 100644 --- a/core/src/main/thrift-gen-java/org/apache/accumulo/core/clientImpl/thrift/ClientService.java +++ b/core/src/main/thrift-gen-java/org/apache/accumulo/core/clientImpl/thrift/ClientService.java @@ -9604,7 +9604,7 @@ public void setPrincipalIsSet(boolean value) { } public byte[] getPassword() { - setPassword(org.apache.thrift.TBaseHelper.rightSize(password)); + this.password = org.apache.thrift.TBaseHelper.rightSize(password); return password == null ? null : password.array(); } @@ -11720,7 +11720,7 @@ public void setPrincipalIsSet(boolean value) { } public byte[] getPassword() { - setPassword(org.apache.thrift.TBaseHelper.rightSize(password)); + this.password = org.apache.thrift.TBaseHelper.rightSize(password); return password == null ? null : password.array(); } diff --git a/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TColumn.java b/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TColumn.java index 6b6ca9de830..1fe6383ff73 100644 --- a/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TColumn.java +++ b/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TColumn.java @@ -163,7 +163,7 @@ public void clear() { } public byte[] getColumnFamily() { - setColumnFamily(org.apache.thrift.TBaseHelper.rightSize(columnFamily)); + this.columnFamily = org.apache.thrift.TBaseHelper.rightSize(columnFamily); return columnFamily == null ? null : columnFamily.array(); } @@ -197,7 +197,7 @@ public void setColumnFamilyIsSet(boolean value) { } public byte[] getColumnQualifier() { - setColumnQualifier(org.apache.thrift.TBaseHelper.rightSize(columnQualifier)); + this.columnQualifier = org.apache.thrift.TBaseHelper.rightSize(columnQualifier); return columnQualifier == null ? null : columnQualifier.array(); } @@ -231,7 +231,7 @@ public void setColumnQualifierIsSet(boolean value) { } public byte[] getColumnVisibility() { - setColumnVisibility(org.apache.thrift.TBaseHelper.rightSize(columnVisibility)); + this.columnVisibility = org.apache.thrift.TBaseHelper.rightSize(columnVisibility); return columnVisibility == null ? null : columnVisibility.array(); } diff --git a/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TCondition.java b/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TCondition.java index afad65f9358..adc312765bb 100644 --- a/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TCondition.java +++ b/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TCondition.java @@ -219,7 +219,7 @@ public void clear() { } public byte[] getCf() { - setCf(org.apache.thrift.TBaseHelper.rightSize(cf)); + this.cf = org.apache.thrift.TBaseHelper.rightSize(cf); return cf == null ? null : cf.array(); } @@ -253,7 +253,7 @@ public void setCfIsSet(boolean value) { } public byte[] getCq() { - setCq(org.apache.thrift.TBaseHelper.rightSize(cq)); + this.cq = org.apache.thrift.TBaseHelper.rightSize(cq); return cq == null ? null : cq.array(); } @@ -287,7 +287,7 @@ public void setCqIsSet(boolean value) { } public byte[] getCv() { - setCv(org.apache.thrift.TBaseHelper.rightSize(cv)); + this.cv = org.apache.thrift.TBaseHelper.rightSize(cv); return cv == null ? null : cv.array(); } @@ -367,7 +367,7 @@ public void setHasTimestampIsSet(boolean value) { } public byte[] getVal() { - setVal(org.apache.thrift.TBaseHelper.rightSize(val)); + this.val = org.apache.thrift.TBaseHelper.rightSize(val); return val == null ? null : val.array(); } @@ -401,7 +401,7 @@ public void setValIsSet(boolean value) { } public byte[] getIterators() { - setIterators(org.apache.thrift.TBaseHelper.rightSize(iterators)); + this.iterators = org.apache.thrift.TBaseHelper.rightSize(iterators); return iterators == null ? null : iterators.array(); } diff --git a/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TKey.java b/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TKey.java index c274da3ab46..c5616641208 100644 --- a/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TKey.java +++ b/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TKey.java @@ -192,7 +192,7 @@ public void clear() { } public byte[] getRow() { - setRow(org.apache.thrift.TBaseHelper.rightSize(row)); + this.row = org.apache.thrift.TBaseHelper.rightSize(row); return row == null ? null : row.array(); } @@ -226,7 +226,7 @@ public void setRowIsSet(boolean value) { } public byte[] getColFamily() { - setColFamily(org.apache.thrift.TBaseHelper.rightSize(colFamily)); + this.colFamily = org.apache.thrift.TBaseHelper.rightSize(colFamily); return colFamily == null ? null : colFamily.array(); } @@ -260,7 +260,7 @@ public void setColFamilyIsSet(boolean value) { } public byte[] getColQualifier() { - setColQualifier(org.apache.thrift.TBaseHelper.rightSize(colQualifier)); + this.colQualifier = org.apache.thrift.TBaseHelper.rightSize(colQualifier); return colQualifier == null ? null : colQualifier.array(); } @@ -294,7 +294,7 @@ public void setColQualifierIsSet(boolean value) { } public byte[] getColVisibility() { - setColVisibility(org.apache.thrift.TBaseHelper.rightSize(colVisibility)); + this.colVisibility = org.apache.thrift.TBaseHelper.rightSize(colVisibility); return colVisibility == null ? null : colVisibility.array(); } diff --git a/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TKeyExtent.java b/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TKeyExtent.java index 808443868f3..0fe958f5592 100644 --- a/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TKeyExtent.java +++ b/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TKeyExtent.java @@ -163,7 +163,7 @@ public void clear() { } public byte[] getTable() { - setTable(org.apache.thrift.TBaseHelper.rightSize(table)); + this.table = org.apache.thrift.TBaseHelper.rightSize(table); return table == null ? null : table.array(); } @@ -197,7 +197,7 @@ public void setTableIsSet(boolean value) { } public byte[] getEndRow() { - setEndRow(org.apache.thrift.TBaseHelper.rightSize(endRow)); + this.endRow = org.apache.thrift.TBaseHelper.rightSize(endRow); return endRow == null ? null : endRow.array(); } @@ -231,7 +231,7 @@ public void setEndRowIsSet(boolean value) { } public byte[] getPrevEndRow() { - setPrevEndRow(org.apache.thrift.TBaseHelper.rightSize(prevEndRow)); + this.prevEndRow = org.apache.thrift.TBaseHelper.rightSize(prevEndRow); return prevEndRow == null ? null : prevEndRow.array(); } diff --git a/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TKeyValue.java b/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TKeyValue.java index 83978596a86..b91f7fbc38c 100644 --- a/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TKeyValue.java +++ b/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TKeyValue.java @@ -175,7 +175,7 @@ public void setKeyIsSet(boolean value) { } public byte[] getValue() { - setValue(org.apache.thrift.TBaseHelper.rightSize(value)); + this.value = org.apache.thrift.TBaseHelper.rightSize(value); return value == null ? null : value.array(); } diff --git a/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TMutation.java b/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TMutation.java index cbaaed190b8..ce95f1c3887 100644 --- a/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TMutation.java +++ b/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TMutation.java @@ -195,7 +195,7 @@ public void clear() { } public byte[] getRow() { - setRow(org.apache.thrift.TBaseHelper.rightSize(row)); + this.row = org.apache.thrift.TBaseHelper.rightSize(row); return row == null ? null : row.array(); } @@ -229,7 +229,7 @@ public void setRowIsSet(boolean value) { } public byte[] getData() { - setData(org.apache.thrift.TBaseHelper.rightSize(data)); + this.data = org.apache.thrift.TBaseHelper.rightSize(data); return data == null ? null : data.array(); } diff --git a/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TRowRange.java b/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TRowRange.java index 39979f393ca..2867a4e81c9 100644 --- a/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TRowRange.java +++ b/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TRowRange.java @@ -150,7 +150,7 @@ public void clear() { } public byte[] getStartRow() { - setStartRow(org.apache.thrift.TBaseHelper.rightSize(startRow)); + this.startRow = org.apache.thrift.TBaseHelper.rightSize(startRow); return startRow == null ? null : startRow.array(); } @@ -184,7 +184,7 @@ public void setStartRowIsSet(boolean value) { } public byte[] getEndRow() { - setEndRow(org.apache.thrift.TBaseHelper.rightSize(endRow)); + this.endRow = org.apache.thrift.TBaseHelper.rightSize(endRow); return endRow == null ? null : endRow.array(); } diff --git a/core/src/main/thrift-gen-java/org/apache/accumulo/core/manager/thrift/ManagerClientService.java b/core/src/main/thrift-gen-java/org/apache/accumulo/core/manager/thrift/ManagerClientService.java index ad6eb39aa1a..17a4858e0f7 100644 --- a/core/src/main/thrift-gen-java/org/apache/accumulo/core/manager/thrift/ManagerClientService.java +++ b/core/src/main/thrift-gen-java/org/apache/accumulo/core/manager/thrift/ManagerClientService.java @@ -7419,7 +7419,7 @@ public void setTableNameIsSet(boolean value) { } public byte[] getStartRow() { - setStartRow(org.apache.thrift.TBaseHelper.rightSize(startRow)); + this.startRow = org.apache.thrift.TBaseHelper.rightSize(startRow); return startRow == null ? null : startRow.array(); } @@ -7453,7 +7453,7 @@ public void setStartRowIsSet(boolean value) { } public byte[] getEndRow() { - setEndRow(org.apache.thrift.TBaseHelper.rightSize(endRow)); + this.endRow = org.apache.thrift.TBaseHelper.rightSize(endRow); return endRow == null ? null : endRow.array(); } diff --git a/core/src/main/thrift-gen-java/org/apache/accumulo/core/securityImpl/thrift/TAuthenticationKey.java b/core/src/main/thrift-gen-java/org/apache/accumulo/core/securityImpl/thrift/TAuthenticationKey.java index bc52759db09..f6011e5044f 100644 --- a/core/src/main/thrift-gen-java/org/apache/accumulo/core/securityImpl/thrift/TAuthenticationKey.java +++ b/core/src/main/thrift-gen-java/org/apache/accumulo/core/securityImpl/thrift/TAuthenticationKey.java @@ -173,7 +173,7 @@ public void clear() { } public byte[] getSecret() { - setSecret(org.apache.thrift.TBaseHelper.rightSize(secret)); + this.secret = org.apache.thrift.TBaseHelper.rightSize(secret); return secret == null ? null : secret.array(); } diff --git a/core/src/main/thrift-gen-java/org/apache/accumulo/core/securityImpl/thrift/TCredentials.java b/core/src/main/thrift-gen-java/org/apache/accumulo/core/securityImpl/thrift/TCredentials.java index 0aaaa0bb19c..7843c9650f0 100644 --- a/core/src/main/thrift-gen-java/org/apache/accumulo/core/securityImpl/thrift/TCredentials.java +++ b/core/src/main/thrift-gen-java/org/apache/accumulo/core/securityImpl/thrift/TCredentials.java @@ -226,7 +226,7 @@ public void setTokenClassNameIsSet(boolean value) { } public byte[] getToken() { - setToken(org.apache.thrift.TBaseHelper.rightSize(token)); + this.token = org.apache.thrift.TBaseHelper.rightSize(token); return token == null ? null : token.array(); } diff --git a/core/src/main/thrift-gen-java/org/apache/accumulo/core/securityImpl/thrift/TDelegationToken.java b/core/src/main/thrift-gen-java/org/apache/accumulo/core/securityImpl/thrift/TDelegationToken.java index 7202be9db75..52b0b244b9b 100644 --- a/core/src/main/thrift-gen-java/org/apache/accumulo/core/securityImpl/thrift/TDelegationToken.java +++ b/core/src/main/thrift-gen-java/org/apache/accumulo/core/securityImpl/thrift/TDelegationToken.java @@ -150,7 +150,7 @@ public void clear() { } public byte[] getPassword() { - setPassword(org.apache.thrift.TBaseHelper.rightSize(password)); + this.password = org.apache.thrift.TBaseHelper.rightSize(password); return password == null ? null : password.array(); } diff --git a/core/src/main/thrift-gen-java/org/apache/accumulo/core/tabletserver/thrift/TabletServerClientService.java b/core/src/main/thrift-gen-java/org/apache/accumulo/core/tabletserver/thrift/TabletServerClientService.java index 8a243255046..6a9f0cda1cf 100644 --- a/core/src/main/thrift-gen-java/org/apache/accumulo/core/tabletserver/thrift/TabletServerClientService.java +++ b/core/src/main/thrift-gen-java/org/apache/accumulo/core/tabletserver/thrift/TabletServerClientService.java @@ -3050,7 +3050,7 @@ public void setTableIdIsSet(boolean value) { } public byte[] getStartRow() { - setStartRow(org.apache.thrift.TBaseHelper.rightSize(startRow)); + this.startRow = org.apache.thrift.TBaseHelper.rightSize(startRow); return startRow == null ? null : startRow.array(); } @@ -3084,7 +3084,7 @@ public void setStartRowIsSet(boolean value) { } public byte[] getEndRow() { - setEndRow(org.apache.thrift.TBaseHelper.rightSize(endRow)); + this.endRow = org.apache.thrift.TBaseHelper.rightSize(endRow); return endRow == null ? null : endRow.array(); } From a24c29e98472d31034d543dc00fca0844aa77a7f Mon Sep 17 00:00:00 2001 From: Christopher Tubbs Date: Mon, 6 Jul 2026 18:06:03 -0400 Subject: [PATCH 3/4] Fix some new Text(null) instances Simplify some TextUtil code --- .../apache/accumulo/core/summary/Gatherer.java | 8 ++++---- .../apache/accumulo/core/util/TextUtil.java | 18 +++++++----------- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/summary/Gatherer.java b/core/src/main/java/org/apache/accumulo/core/summary/Gatherer.java index 0ba937d8655..1665b1e1a15 100644 --- a/core/src/main/java/org/apache/accumulo/core/summary/Gatherer.java +++ b/core/src/main/java/org/apache/accumulo/core/summary/Gatherer.java @@ -126,8 +126,8 @@ public Gatherer(ClientContext context, TSummaryRequest request, AccumuloConfigur CryptoService cryptoService) { this.ctx = context; this.tableId = TableId.of(request.getTableId()); - this.startRow = new Text(request.getBounds().getStartRow()); - this.endRow = new Text(request.getBounds().getEndRow()); + 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()); @@ -435,8 +435,8 @@ public Future processFiles(FileSystemResolver volMgr, Map> files, BlockCache summaryCache, BlockCache indexCache, Cache fileLenCache, ExecutorService srp) { Function fromThrift = tRowRange -> { - Text lowerBound = new Text(tRowRange.getStartRow()); - Text upperBound = new Text(tRowRange.getEndRow()); + Text lowerBound = TextUtil.fromNullableBytes(tRowRange.getStartRow()); + Text upperBound = TextUtil.fromNullableBytes(tRowRange.getEndRow()); return RowRange.range(lowerBound, false, upperBound, true); }; List> futures = new ArrayList<>(); diff --git a/core/src/main/java/org/apache/accumulo/core/util/TextUtil.java b/core/src/main/java/org/apache/accumulo/core/util/TextUtil.java index cbdcd0321a2..463f7475112 100644 --- a/core/src/main/java/org/apache/accumulo/core/util/TextUtil.java +++ b/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; } From 530e91aff94f0a0d898f633a9893d14a382f7bf4 Mon Sep 17 00:00:00 2001 From: Christopher Tubbs Date: Mon, 6 Jul 2026 18:35:31 -0400 Subject: [PATCH 4/4] Add preventative script --- .github/workflows/maven.yaml | 2 + .../ci/find-unapproved-thrift-bytebuffer.sh | 51 +++++++++++++++++++ 2 files changed, 53 insertions(+) create mode 100755 src/build/ci/find-unapproved-thrift-bytebuffer.sh diff --git a/.github/workflows/maven.yaml b/.github/workflows/maven.yaml index a42e83d764d..5f0e41cd0a4 100644 --- a/.github/workflows/maven.yaml +++ b/.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 diff --git a/src/build/ci/find-unapproved-thrift-bytebuffer.sh b/src/build/ci/find-unapproved-thrift-bytebuffer.sh new file mode 100755 index 00000000000..bb6068070bf --- /dev/null +++ b/src/build/ci/find-unapproved-thrift-bytebuffer.sh @@ -0,0 +1,51 @@ +#! /usr/bin/env bash +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + +# The purpose of this ci script is to ensure that a pull request doesn't +# unintentionally add any new calls to Thrift generated methods of the +# form "bufferForX()" which create expensive unneeded byte array copies. +NUM_EXPECTED=0 +ALLOWED=( +) + +ALLOWED_PIPE_SEP=$({ for x in "${ALLOWED[@]}"; do echo "$x"; done; } | paste -sd'|') + +function findallbufferfor() { + # -P for perl matching, -R for recursive, -l for matching files + local opts='-PRl' + if [[ $1 == 'print' ]]; then + # -P for perl matching, -R for recursive, -l for matching files, -H for always showing filenames + opts='-PRlH' + fi + # find any new classes using something other than the jupiter API, except those allowed + grep "$opts" --include='*.java' '[.]bufferFor[^(]+[(]' | grep -Pv "^(${ALLOWED_PIPE_SEP//./[.]})\$" +} + +function comparecounts() { + local count + count=$(findallbufferfor | wc -l) + if [[ $NUM_EXPECTED -ne $count ]]; then + echo "Expected $NUM_EXPECTED, but found $count classes using 'bufferForX()' calls:" + findallbufferfor 'print' + return 1 + fi +} + +comparecounts && echo "Found exactly $NUM_EXPECTED unapproved 'bufferForX()' calls, as expected"