PHOENIX-7003 Harden hbase region inconsistencies check in CQSI#getAllTableRegions method #1686
Conversation
Divneet18
changed the title
There was a problem hiding this comment.
Please update the commit message to conform the the Phoenix conventions, i.e.
"PHOENIX-7003 Harden hbase region inconsistencies check in CQSI#getAllTableRegions method"
Divneet18
changed the title
|
@Divneet18 Istvan is asking to change commit message (which is here). You changed the title and that is also required. |
| @@ -194,7 +194,28 @@ public void testGetNextRegionStartKey() { | |||
| GlobalClientMetrics.GLOBAL_HBASE_COUNTER_METADATA_INCONSISTENCY.getMetric().reset(); | |||
| when(mockHRegionInfo.getStartKey()).thenReturn(notCorruptedStartKey); | |||
| when(mockHRegionInfo.getEndKey()).thenReturn(notCorruptedNewKey); | |||
| testGetNextRegionStartKey(mockCqsi, mockRegionLocation, notCorruptedEndKey, false); | |||
| testGetNextRegionStartKey(mockCqsi, mockRegionLocation, notCorruptedEndKey, true); | |||
There was a problem hiding this comment.
Why is isCorrupted true here if both start and endkey are not corrupt and the current endkey is greater than the previous end key?
There was a problem hiding this comment.
for the new region, the startKey in this case is 0x2999 and endKey is 0x3001. The previous regions end key is 0x3000 leading to an overlap. Hence, isCorrupted is true here. However, your confusion makes sense because the variable names were incorrect. I have updated the variable names to start with "corrupted" in all cases where isCorrupted is true and "notCorrupted" otherwise. I have pushed with new changes. Please review!
| @@ -707,7 +707,8 @@ public void clearTableRegionCache(TableName tableName) throws SQLException { | |||
| public byte[] getNextRegionStartKey(HRegionLocation regionLocation, byte[] currentKey) throws IOException { | |||
| // in order to check the overlap/inconsistencies bad region info, we have to make sure | |||
| // the current endKey always increasing(compare the previous endKey) | |||
| if (Bytes.compareTo(regionLocation.getRegion().getEndKey(), currentKey) <= 0 | |||
| if ((Bytes.compareTo(regionLocation.getRegion().getStartKey(), currentKey) != 0 | |||
There was a problem hiding this comment.
just trying to understand what does !=0 imply? => startKey could be less than currentKey or greater than currentKey. Is that the real intention or should it specific either less than or greater than?
Also should currentKey be changed to currentEndKey to be inline with the comments and the actual intent of it?
There was a problem hiding this comment.
Whenever the start key of new region is not same as the end key of the previous region it will cause an overlap or a hole. Thats why when the compareTo condition is !=0 we want to throw an error.
the currentKey is the previous regions endKey being used in the getAllTableRegions method. Its being called currentKey because of how getAllTableRegions iterates over the list and compares but to avoid further confusion I have added a comment in the code mentioning currentKey=previous regions endKey
There was a problem hiding this comment.
ok, got it. I was thinking of the reverse condition :) Thanks for clarifying.
…TableRegions method
Divneet18
force-pushed
the
PHOENIX-7004
branch
from
df4c60f
to
3d4772b
Compare
…SI#getAllTableRegions method) based on comments
|
Unless there is any objections, let me merge this after 24 hr. Thank you @Divneet18! |
|
@Divneet18 can you please create another PR against 5.1 branch? |
…TableRegions method (#1686)
|
no worries, i have done the cherry-pick @Divneet18, thanks again! |
Changing the region boundary detection conditions