Revert "Use private Thrift members (#6430)"

[dlmarion]

GarbageCollectorIT.gcLotsOfCandidatesIT has consistently failed on a server since #6430 was merged. The failure happens when the garbage collector process is started with 32 MB of memory. This PR branch exists so that I can test this on that server.

This reverts commit bdcd798.

[dlmarion]

I think I found a real issue with #6430 and we might want to actually revert it. The original code would access a ByteBuffer directly. #6430 made those member variables private and the methods for accessing the ByteBuffer's make a copy before returning them.

[dlmarion]

Here's a good example of the impact of this change.

accumulo/core/src/main/java/org/apache/accumulo/core/clientImpl/TabletServerBatchReaderIterator.java

Lines 957 to 960 in 1234b63

	ArrayList<Entry<Key,Value>> entries = new ArrayList<>(scanResult.getResults().size());
	for (TKeyValue kv : scanResult.getResults()) {
	entries.add(new SimpleImmutableEntry<>(new Key(kv.getKey()), new Value(kv.getValue())));
	}

The current version of Key(TKey) looks like:

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

Lines 687 to 707 in 1234b63

	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.timestamp = tkey.getTimestamp();
	this.deleted = false;
	if (row == null) {
	throw new IllegalArgumentException("null row");
	}
	if (colFamily == null) {
	throw new IllegalArgumentException("null column family");
	}
	if (colQualifier == null) {
	throw new IllegalArgumentException("null column qualifier");
	}
	if (colVisibility == null) {
	throw new IllegalArgumentException("null column visibility");
	}
	}

The same code prior to #6430 looks like:

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

Lines 688 to 707 in 1852dbf

	public Key(TKey tkey) {
	this.row = toBytes(tkey.row);
	this.colFamily = toBytes(tkey.colFamily);
	this.colQualifier = toBytes(tkey.colQualifier);
	this.colVisibility = toBytes(tkey.colVisibility);
	this.timestamp = tkey.timestamp;
	this.deleted = false;
	if (row == null) {
	throw new IllegalArgumentException("null row");
	}
	if (colFamily == null) {
	throw new IllegalArgumentException("null column family");
	}
	if (colQualifier == null) {
	throw new IllegalArgumentException("null column qualifier");
	}
	if (colVisibility == null) {
	throw new IllegalArgumentException("null column visibility");
	}

The calls to bufferForX create a copy of the ByteBuffer.

[ctubbsii]

I'm currently looking into alternatives. I think the private thrift members is a nice change, but I also recognize the problem with the extra protective copy. Please hold on merging this for now, while I investigate.

[ctubbsii]

I created #6457 as an alternative to reverting all this. It will remove the unwanted protective copies that are likely causing the performance problem.

[ctubbsii]

I'd prefer to resolve the issue with #6457 instead, if works.