DomGarguilo commented on code in PR #6430:
URL: https://github.com/apache/accumulo/pull/6430#discussion_r3431265424
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletStatsKeeper.java:
##########
@@ -38,20 +38,21 @@ public void updateTime(Operation operation, long queued,
long start, long count,
try {
ActionStats data = map[operation.ordinal()];
if (failed) {
- data.fail++;
- data.status--;
+ data.setFail(data.getFail() + 1);
+ data.setStatus(data.getStatus() - 1);
} else {
double t = (System.currentTimeMillis() - start) / 1000.0;
double q = (start - queued) / 1000.0;
- data.status--;
- data.count += count;
- data.num++;
- data.elapsed += t;
- data.queueTime += q;
- data.sumDev += t * t;
- data.queueSumDev += q * q;
- if (data.elapsed < 0 || data.sumDev < 0 || data.queueSumDev < 0 ||
data.queueTime < 0) {
+ data.setStatus(data.getStatus() - 1);
+ data.setCount(data.getCount() + count);
+ data.setNum(data.getNum() + 1);
+ data.setElapsed(data.getElapsed() + t);
+ data.setQueueTime(data.getQueueTime() + q);
+ data.setSumDev(data.getSumDev() + t * t);
+ data.setQueueTime(data.getQueueTime() + q * q);
Review Comment:
```suggestion
data.setQueueSumDev(data.getQueueSumDev() + q * q);
```
##########
core/src/main/java/org/apache/accumulo/core/iteratorsImpl/IteratorConfigUtil.java:
##########
@@ -215,21 +215,21 @@ public static SortedKeyValueIterator<Key,Value>
convertItersAndLoad(IteratorScop
for (IterInfo iterInfo : iteratorBuilder.iters) {
Class<SortedKeyValueIterator<Key,Value>> clazz = null;
- log.trace("Attempting to load iterator class {}", iterInfo.className);
+ log.trace("Attempting to load iterator class {}",
iterInfo.getClassName());
if (iteratorBuilder.useClassCache) {
- clazz = classCache.get(iterInfo.className);
+ clazz = classCache.get(iterInfo.getIterName());
if (clazz == null) {
clazz = loadClass(useClassLoader, iteratorBuilder.context,
iterInfo);
- classCache.put(iterInfo.className, clazz);
+ classCache.put(iterInfo.getIterName(), clazz);
Review Comment:
```suggestion
classCache.put(iterInfo.getClassName(), clazz);
```
##########
core/src/main/java/org/apache/accumulo/core/iteratorsImpl/IteratorConfigUtil.java:
##########
@@ -215,21 +215,21 @@ public static SortedKeyValueIterator<Key,Value>
convertItersAndLoad(IteratorScop
for (IterInfo iterInfo : iteratorBuilder.iters) {
Class<SortedKeyValueIterator<Key,Value>> clazz = null;
- log.trace("Attempting to load iterator class {}", iterInfo.className);
+ log.trace("Attempting to load iterator class {}",
iterInfo.getClassName());
if (iteratorBuilder.useClassCache) {
- clazz = classCache.get(iterInfo.className);
+ clazz = classCache.get(iterInfo.getIterName());
Review Comment:
```suggestion
clazz = classCache.get(iterInfo.getClassName());
```
##########
core/src/main/java/org/apache/accumulo/core/data/Column.java:
##########
@@ -139,7 +138,7 @@ public Column(byte[] columnFamily, byte[] columnQualifier,
byte[] columnVisibili
* @param tcol Thrift column
*/
public Column(TColumn tcol) {
- this(toBytes(tcol.columnFamily), toBytes(tcol.columnQualifier),
toBytes(tcol.columnVisibility));
+ this(tcol.getColumnFamily(), tcol.getColumnQualifier(),
tcol.getColumnVisibility());
Review Comment:
```suggestion
this(toBytes(tcol.bufferForColumnFamily()),
toBytes(tcol.bufferForColumnQualifier()),
toBytes(tcol.bufferForColumnVisibility()));
```
I think we should keep the `toBytes(...)` conversion anywhere we’re copying
Thrift binary fields into accumulo objects that store `byte[]`. The generated
`getX()` methods can expose the Thrift object’s backing array, while
`bufferForX()` returns a copied buffer. So the safer pattern is
`ByteBufferUtil.toBytes(x.bufferForY())`. It’s uglier, but it preserves the old
defensive-copy behavior.
--
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]
