PHOENIX-2417 Compress memory used by row key byte[] of guideposts

[ankitsinghal]

• Still need to tweak some copying of bytes
• Having only these two test case failing currently
  ViewIT.testNonSaltedUpdatableViewWithIndex:129->BaseViewIT.testUpdatableViewWithIndex:85->BaseViewIT.testUpdatableViewIndex:158 expected:<6> but was:<2>
  ViewIT.testNonSaltedUpdatableViewWithIndex:129->BaseViewIT.testUpdatableViewWithIndex:85->BaseViewIT.testUpdatableViewIndex:158 expected:<6> but was:<2>

[JamesRTaylor]

You can't treat an ImmutableBytesWritable the same as what was a byte[] before because an ImmutableBytesWritable has an offset and a length. By doing it this way, you're assuming that the offset is 0 and the length is byte[].length. Instead, you'd want to change the type and adjust the code as necessary:

ImmutableBytesWritable originalStartKey= originalStartKeyPtr;
ImmutableBytesWritable originalStopKey = originalStopKeyPtr;

[JamesRTaylor]

Thanks for the patch, @ankitsinghal. The main remaining issues are around not treating an ImmutableBytesWritable the same as a byte[]. Maybe easiest for now to have BaseResultIterators.getParallelScans() declare currentKey as an ImmutableBytesWritable, but keep

// Do this as infrequently as possible to prevent a copy of the backing byte array
byte[] currentKeyBytes = SchemaUtil.copyKeyIfNecessary(currentKey);
byte[] currentGuidePostBytes = SchemaUtil.copyKeyIfNecessary(currentGuidePost);
Scan newScan = scanRanges.intersectScan(scan, currentKeyBytes, currentGuidePostBytes, keyOffset, false);
scans = addNewScan(parallelScans, scans, newScan, currentGuidePostBytes, false, regionLocation);

[ankitsinghal]

Thanks @JamesRTaylor for the review.
I have made the changes you have suggested above except this one.
byte[] currentGuidePostBytes = SchemaUtil.copyKeyIfNecessary(currentGuidePost);

As PrefixByteDecoder updates the previous buffer only whenever maxLength is passed as a part of optimization.

public ImmutableBytesWritable decode(DataInput in) throws IOException {
int prefixLen = WritableUtils.readVInt(in);
int suffixLen = WritableUtils.readVInt(in);
int length = prefixLen + suffixLen;
byte[] b;
if (maxLength == -1) { // Allocate new byte array each time
b = new byte[length];
System.arraycopy(previous.get(), previous.getOffset(), b, 0, prefixLen);
} else { // Reuse same buffer each time
b = previous.get();
}
in.readFully(b, prefixLen, suffixLen);
previous.set(b, 0, length);
return previous;
}

so I need to copy bytes even if the length of the ImmutableByteWritable is equal to byte[] contained in it.

What do you think about this?

And also I have fixed the failure test cases as well. there was logical operator problem while incrementing
guidePosts till the start key.

I have run the complete test suite now and will confirm you once it is completed.

[JamesRTaylor]

This might be a b/w compat issue. Safest would be to add guidePosts at the end and keep values as-is.

[JamesRTaylor]

There's a pretty big backward compatibility issue due to PHOENIX-2143 and this one. The case you'll need to make work is an old pre 4.7.0 client that's running against a new 4.7.0 server. The client will expect the stats to be in the original format. In the following call:

public void getTable(RpcController controller, GetTableRequest request,
RpcCallback<MetaDataResponse> done) {

You'll need to pass request.getClientVersion() through doGetTable(), into getTable() and finally into StatisticsUtil.readStatistics(). You should preserve the old code (we can dump it when we do a major release), and use that code path if the stats have not been regenerated yet. You can detect this based on the existence of the GUIDE_POSTS key value (which you'll want to project into the scan for the new code for this b/w compatibility case). If the stats have been regenerated, there'd be two cases: the client is pre 4.7.0 in which case you'd want to use the new code but put the data in the old format, or the client is 4.7.0 or above in which case your existing code is fine.

With PHOENIX-2143, when compaction runs, we'll generate stats in the new format. It's possible that the SYSTEM.STATS table hasn't been updated yet (as this gets triggered when a new 4.7.0 client connects to the server which may not yet have happened). We'd need to issue the previous Delete marker based on the old row key structure to ensure that the stats for the region are deleted. We wouldn't want to issue the query that does the range delete in this case because it might delete rows across multiple regions (ugh). So we'd need to know if the schema upgrade has been done yet when compaction runs. We could detect this by querying the SYSTEM.CATALOG table directly or by using the MetaDataProtocol.getTable() call and pulling over the PTable and then conditionally do the delete the old way versus the new way.

WDYT, @ankitsinghal? A more radical alternative would be to call this release 5.0. Users could still upgrade the server and client as with a minor release, but they'd need to truncate the SYSTEM.STATS table manually before upgrading the server. In that case, I think it'd be acceptable to return an empty guidepost for the protobuf values field (as essentially stats would be disabled for older clients running against the newer server).

[JamesRTaylor]

I'm leaning toward having this release be 5.0. We can add some code MetaDataRegionObserver.postOpen() that does a checkAndPut on the SYSTEM.CATALOG table for the SYSTEM.STATS row where we conditionally truncate the table (i.e. invoke the code you already wrote).

[JamesRTaylor]

Let's do something in-the-middle. We can stick with the plan that this is still 4.7.0 release, but we can do the above in MetaDataRegionObserver to ensure that the SYSTEM.STATS table is truncated. Here what needs to be done:

• conditionally truncate SYSTEM.STATS table in MetaDataRegionObserver.postOpen() based on checkAndPut
• keep values field at protobuf position 2 and return an empty PGuidePosts for that field. We'll document that stats are essentially disabled for an old client once you upgrade your server (but nothing will break).
• lastly (unrelated to b/w compat), create a GuidePostsInfoWriter class (or StatisticsUtil method) and move the GuidePostsInfo.encodeAndCollectGuidePost() logic there.

[ankitsinghal]

yes it makes sense, as truncate of system.stats is necessary. I'll try this tomorrow.

Yes ,as per your previous comments , I'll be keeping "values" field at position 2, so this will automatically ensure that empty PGuidePosts is returned when client older than 4.7 is used right?
As per below code from version older than 4.7 (PTableImpl.createFromProto() )
GuidePostsInfo info =
new GuidePostsInfo(guidePostsByteCount, value, rowCount);//Prior 4.7 version :- empty "value" list.

3. I have created a GuidePostsInfoWriter and you can review the changes in this pull request now.

[JamesRTaylor]

Looks good functionally now, @ankitsinghal. Just needs a little bit of cleanup. Nice work!

[JamesRTaylor]

The if isn't needed because the client-side code that truncates the stats table has been removed. This is the only place we do it. If stats building gets triggered before the client-side upgrade code has run (for example, through compaction), then it will build using the new logic with the new schema. It should be fine, b/c the code uses straight HBase APIs, not Phoenix APIs. Since we always send back empty guideposts for the protobuf field that old clients will be looking at, clients will just get empty stats until the client-side upgrade code runs. We should confirm this with manual testing.

[JamesRTaylor]

Actually, I take it back. Your way is better.

[ankitsinghal]

Thanks @JamesRTaylor for all the review and help.