PHOENIX-5564 Restructure read repair to improve readability and corre… #624
Conversation
phoenix-core/src/main/java/org/apache/phoenix/index/GlobalIndexChecker.java
Outdated
Show resolved
Hide resolved
| */ | ||
| public class GlobalIndexChecker implements RegionCoprocessor, RegionObserver { | ||
| private static final Log LOG = LogFactory.getLog(GlobalIndexChecker.class); | ||
| private GlobalIndexCheckerSource metricsSource; | ||
| public static final int NO_DATA_ROW = 0; |
There was a problem hiding this comment.
Should this be an enum, esp since it's used in the Unaggregated coproc as well?
There was a problem hiding this comment.
(Note that this is a genuine question, not just requesting a change politely. :-) )
| // so that it can be repaired | ||
| scanner.close(); | ||
| scanner = region.getScanner(indexScan); | ||
| row.clear(); |
There was a problem hiding this comment.
Do we not need to return here? We're clearing the scanned row but in the very next block we're checking if it still exists after a new rebuild (and I think the next block is assuming that it's an else{} to this one). Otherwise I don't see where the rebuild of the "new" unverified different-keyed-row is happening.
There was a problem hiding this comment.
The reason this missing return did not cause any issue is because the row is cleared and the next block of code returns when the row is empty (i.e., verifyRowAndRemoveEmptyColumn(row) returns true when the row is empty):
if (verifyRowAndRemoveEmptyColumn(row)) {
// The index row status is "verified". This row is good to return to the client. We are done here.
return;
}
phoenix-core/src/main/java/org/apache/phoenix/index/GlobalIndexChecker.java
Show resolved
Hide resolved
| long rowCount = PLong.INSTANCE.getCodec().decodeLong(new ImmutableBytesWritable(value), SortOrder.getDefault()); | ||
| if (rowCount == 0) { | ||
| // This means there does not exist a data table row for this unverified index row | ||
| long code = PLong.INSTANCE.getCodec().decodeLong(new ImmutableBytesWritable(value), SortOrder.getDefault()); |
There was a problem hiding this comment.
Rather than doing getValue a bunch, probably better to translate code into the enum once and then refer to it directly.
There was a problem hiding this comment.
I appreciate if you give me an example here to follow.
There was a problem hiding this comment.
So, it turns out it's messier to translate a long to an enum than an int, so I leave it up to you. But something like:
long result = PLong.INSTANCE.getCodec().decodeLong(new ImmutableBytesWritable(value), SortOrder.getDefault());
RebuildReturnCode returnCode = RebuildReturnCode.values[(Integer) result];
The idea was that you'd have a variable of type RebuildReturnCode, so you wouldn't have to keep converting enum values to longs in multiple places in the code and could just compare the result against the enum values directly. But the narrowing cast makes it a bit messy.
| indexRowKey, 0, indexRowKey.length) == 0); | ||
| // This should not happen at all | ||
| Cell cell = row.get(0); | ||
| byte[] rowKey = new byte[cell.getRowLength()]; |
There was a problem hiding this comment.
Can use CellUtil.cloneRow()
…ctness