PHOENIX-5676 Inline-verification from IndexTool does not handle TTL/r… #685
Conversation
| @@ -584,16 +633,19 @@ public boolean next(List<Cell> results) throws IOException { | |||
| if (onlyVerify) { | |||
| addToBeVerifiedIndexRows(); | |||
| } | |||
| ArrayList<KeyRange> keys = new ArrayList<>(rowCountPerTask); | |||
| ArrayList<KeyRange> keyRanges = new ArrayList<>(rowCountPerTask); | |||
| Set<byte[]> indexRowKeys = new HashSet<>(rowCountPerTask); | |||
There was a problem hiding this comment.
A byte array hash set would not work here. The byte array needs to be wrapped with equals and hashCode, or Map<byte[], Put> created with Maps.newTreeMap(Bytes.BYTES_COMPARATOR) can be used instead.
abhishek-chouhan
force-pushed
the
PHOENIX-5676
branch
from
2760e9d
to
a27b277
Compare
|
Sorry for that miss. Have updated to use TreeSet with appropriate comparator @kadirozde and verified the expected behavior. |
| HColumnDescriptor desc = admin.getTableDescriptor(indexTable).getColumnFamilies()[0]; | ||
| desc.setTimeToLive(1); | ||
| admin.modifyColumn(indexTable, desc); | ||
| Thread.sleep(1000); |
There was a problem hiding this comment.
why not use EEM to advance time instead of sleeping?
There was a problem hiding this comment.
modify is async operation, which needs to actually disable/enable regions. Faking a skip in time would not help.
There was a problem hiding this comment.
is it not sleeping because the test would want to expire the row?
There was a problem hiding this comment.
It is mainly for the admin action but solves both the things. For majority of the cases the admin operation finishes in 1 sec so the retry loop is skipped altogether.
There was a problem hiding this comment.
- Why do you need the first sleep for 1s if you're going to loop to wait later anyway?
- Why is each sleep so long (2s) rather than something short, like 10 or 100ms?
There was a problem hiding this comment.
The first sleep was handling both the things of ttl expiry as well as giving reasonable time for most cases of the admin operation to succeed. The alternative is to use edge just for the ttl of 1 second but still have the retries in place since we actually need to wait for the admin operation. No particular reason of having the retry interval large, thought 40 seconds overall for the test should be enough to have it running reasonably deterministic on the most flaky of hardwares. Let me know if you strongly feel about changing these and i can work on them.
| rowCount++; | ||
| } | ||
| } catch (Throwable t) { | ||
| ServerUtil.throwIOException(indexHTable.getName().toString(), t); | ||
| } | ||
| // Check if any expected rows from index(which we didn't get) are already expired due to TTL | ||
| // TODO: metrics for expired rows |
There was a problem hiding this comment.
Could you please also create a jira for this so that we don't lose it in the comments? Thanks.
There was a problem hiding this comment.
Sure, will do.
There was a problem hiding this comment.
It would be nice to link it to the code's comment.
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
Outdated
Show resolved
Hide resolved
abhishek-chouhan
force-pushed
the
PHOENIX-5676
branch
from
a27b277
to
7661ed6
Compare
abhishek-chouhan
force-pushed
the
PHOENIX-5676
branch
from
7661ed6
to
ef55a2b
Compare
|
Thanks for the reviews! |
…ow-expiry