Fix DbKvs.getRangeOfTimestamps() only returning the first page #1872
Conversation
|
This fix doesn't actually work, please don't merge this yet. |
The expectation is to return all results that fall into the range regardless of the page size. This is how all other KVSs work at least, and also consistent with getRange(). I'm planning to delete all of this code soon, and just need it to work in order to execute the intermediate steps of my big sweep refactor/rewrite.
Codecov Report
@@ Coverage Diff @@
## develop #1872 +/- ##
=============================================
- Coverage 61.36% 61.32% -0.04%
- Complexity 3972 4317 +345
=============================================
Files 740 740
Lines 36832 36851 +19
Branches 3985 3987 +2
=============================================
- Hits 22601 22598 -3
- Misses 12761 12781 +20
- Partials 1470 1472 +2
Continue to review full report at Codecov.
|
|
Pushed a proper fix, changed the test, and updated the PR description. |
|
Right, I was initially a little puzzled as to how adding 1 to the query sizes worked. I'll start reviewing this now. |
|
@jeremyk-91 The logic to determine whether we reached the end was to to check if there was a remaining result from the SQL query, so my idea was to load that extra result. A pretty common trick for implementing paging. It didn't work because "range.getBatchHint()" and the "batchSize" are two different numbers. |
| moreResults = true; | ||
| token = ImmutableToken.builder().row(nextRow).shouldSkip(false).build(); | ||
| } | ||
| } |
There was a problem hiding this comment.
We could extract this to buildTokenIfMoreRowsAvailable or something along those lines.
There was a problem hiding this comment.
We could but I'm planning to delete all of this code soon.
| assertEquals(3, results.size()); | ||
| assertTrue(Arrays.equals(row0, results.get(0).getRowName())); | ||
| assertTrue(Arrays.equals(row1, results.get(1).getRowName())); | ||
| assertTrue(Arrays.equals(row2, results.get(2).getRowName())); |
There was a problem hiding this comment.
For these, I'd prefer we use assertArrayEquals(expected, actual) instead. We'll get a more helpful assertion failure message if something breaks here.
There was a problem hiding this comment.
Yep, forgot that it existed. Done.
| Cell.create(row0, column0), value0_t0, | ||
| Cell.create(row1, column0), value0_t0, | ||
| Cell.create(row2, column0), value0_t0), | ||
| TEST_TIMESTAMP); |
There was a problem hiding this comment.
Ah, the above needs to be indented 4 more spaces in b/c of checkstyle.
[00:02:37] [ant:checkstyle] [ERROR] /home/ubuntu/atlasdb-internal-testing/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/keyvalue/impl/AbstractKeyValueServiceTest.java:1084: 'ImmutableMap' have incorrect indentation level 12, expected level should be 16. [Indentation]
[00:02:37] [ant:checkstyle] [ERROR] /home/ubuntu/atlasdb-internal-testing/atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/keyvalue/impl/AbstractKeyValueServiceTest.java:1088: 'TEST_TIMESTAMP' have incorrect indentation level 12, expected level should be 16. [Indentation]
There was a problem hiding this comment.
Are you guys using the "baseline" checkstyle? I feel like at least half of the rules there don't make much sense.
There was a problem hiding this comment.
For the most part I think so, though we do exclude a couple of rules - @gsheasby would probably know this better!
There was a problem hiding this comment.
@gbonik yep, we recently upgraded to Baseline 0.13.0.
The expectation is to return all results that fall into the range
regardless of the page size. This is how all other KVSs work
at least, and also consistent with getRange().
I'm planning to delete all of this code soon, and just need it to "work" in order
to execute the intermediate steps of my big sweep refactor/rewrite.
Goals (and why):
Fix broken stuff that is blocking my work
Implementation Description (bullets):
Load an extra row. Add a test.
Concerns (what feedback would you like?):
It's still wrong, but that's okay (read above)
Where should we start reviewing?:
Priority (whenever / two weeks / yesterday):
This change is