DbKvs sweep refactors to address last PR comments #2497

gbonik · 2017-10-17T02:47:27Z

As dxiao pointed out, CandidatePagingState does too many things. The
solution is to make CellTsPairLoader return an iterator and move all
paging logic inside that iterator, and remove CellTsPairLoaderFactory.
Then we replace CandidatePagingState with CandidateGroupingIterator,
which now doesn't have any paging logic and only takes care of grouping
(cell, ts) pairs by cell.

Also remove CandidatePageJoiningIterator because it's not used anymore.
We could have done this earlier but I forgot.

Plan for the future:

Since we gave up on the in-database filtering idea, we can replace
KVS.getCandidateCellsForSweeping() with a simpler call like
getCellTsPairs() which would do what CellTsPairLoader does now.
Implement the new call for the remaining KVS's. We need at least
the InMemoryKVS. We should decide the destiny of JdbcKVS and CqlKVS.
Remove all remaining usages of getRangeOfTimestamps(). I think that's
basically deleteRange() on Cassandra.
Remove KVS.getRangeOfTimestamps()!

[no release notes]

Priority (whenever / two weeks / yesterday):
Ideally before we cut for the LTS

This change is

As dxiao pointed out, CandidatePagingState does too many things. The solution is to make CellTsPairLoader return an iterator and move all paging logic inside that iterator, and remove CellTsPairLoaderFactory. Then we replace CandidatePagingState with CandidateGroupingIterator, which now doesn't have any paging logic and only takes care of grouping (cell, ts) pairs by cell. Also remove CandidatePageJoiningIterator because it's not used anymore. We could have done this earlier but I forgot. Plan for the future: - Since we gave up on the in-database filtering idea, we can replace KVS.getCandidateCellsForSweeping() with a simpler call like getCellTsPairs() which would do what CellTsPairLoader does now. - Implement the new call for the remaining KVS's. We need at least the InMemoryKVS. We should decide the destiny of JdbcKVS and CqlKVS. - Remove all remaining usages of getRangeOfTimestamps(). I think that's basically deleteRange() on Cassandra. - Remove KVS.getRangeOfTimestamps()! [no release notes]

codecov-io · 2017-10-17T03:12:27Z

Codecov Report

Merging #2497 into develop will decrease coverage by 0.08%.
The diff coverage is 68.29%.

@@              Coverage Diff              @@
##             develop    #2497      +/-   ##
=============================================
- Coverage       60.1%   60.02%   -0.09%     
+ Complexity      4411     4043     -368     
=============================================
  Files            857      857              
  Lines          40033    40023      -10     
  Branches        4079     4076       -3     
=============================================
- Hits           24062    24023      -39     
- Misses         14503    14532      +29     
  Partials        1468     1468

Impacted Files	Coverage Δ	Complexity Δ
...alue/dbkvs/impl/oracle/OracleCellTsPageLoader.java	`0% <0%> (ø)`	`0 <0> (?)`
.../dbkvs/impl/postgres/PostgresCellTsPageLoader.java	`98.94% <100%> (ø)`	`4 <1> (?)`
...asdb/keyvalue/dbkvs/impl/sweep/CellTsPairInfo.java	`100% <100%> (ø)`	`1 <1> (?)`
...ue/dbkvs/impl/sweep/CandidateGroupingIterator.java	`100% <100%> (ø)`	`16 <16> (?)`
.../impl/sweep/DbKvsGetCandidateCellsForSweeping.java	`100% <100%> (+10%)`	`4 <4> (ø)`	⬇️
...om/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java	`84.35% <50%> (ø)`	`137 <0> (ø)`	⬇️
...sdb/keyvalue/dbkvs/impl/sweep/CellTsPairToken.java	`94.11% <94.11%> (ø)`	`7 <7> (?)`
...main/java/com/palantir/paxos/PaxosLearnerImpl.java	`87.8% <0%> (-2.44%)`	`0% <0%> (ø)`
...alantir/lock/client/LockRefreshingLockService.java	`58.82% <0%> (-1.48%)`	`10% <0%> (ø)`
.../java/com/palantir/util/debug/StackTraceUtils.java	`51.12% <0%> (-1.35%)`	`22% <0%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb998da...34dfa39. Read the comment docs.

gsheasby

To speed things along towards the LTS, I'm going to do the renames and potentially some further refactors myself.

Some questions might remain:

Do we care that we go through an Iterator twice instead of once in order to make the code cleaner?
Does the Iterator returned by DbKvs.getCandidateCellsForSweeping do an eager page load of the kind we were trying to avoid in OracleCellTsPageLoader.PageIterator?

gsheasby · 2017-10-17T11:09:57Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java

@@ -204,7 +204,7 @@ private static DbKvs createOracle(ExecutorService executor,
        OverflowValueLoader overflowValueLoader = new OracleOverflowValueLoader(oracleDdlConfig, tableNameGetter);
        DbKvsGetRange getRange = new OracleGetRange(
                connections, overflowValueLoader, tableNameGetter, valueStyleCache, oracleDdlConfig);
-        CellTsPairLoaderFactory cellTsPairLoaderFactory = new OracleCellTsPageLoaderFactory(
+        CellTsPairLoader cellTsPairLoaderFactory = new OracleCellTsPageLoader(


nit: rename

gsheasby · 2017-10-17T11:22:16Z

...vs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleCellTsPageLoader.java

+                    query.getArgs());
+        }
+
+        private void updateCountOfExaminedCellTsPairsInCurrentRow(List<CellTsPairInfo> results) {


We're iterating through the results twice - once to bundle each one into a CellTsPairInfo, and again to figure out the count of cellTsPairsAlreadyExaminedInCurrentRow.

Is it worth cutting this iteration time in half by updating cellTsPairsAlreadyExaminedInCurrentRow within loadPage()?

gsheasby · 2017-10-17T11:24:02Z

...vs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleCellTsPageLoader.java

+                    if (nextRow == null) {
+                        reachedEnd = true;
+                    } else {
+                        startRowInclusive = nextRow;


These fields go together very tightly. Consider extracting a Token class to cover startRowInclusive, startColInclusive, startTsInclusive, reachedEnd, and cellTsPairsAlreadyExaminedInCurrentRow.

Maybe not cells..currentRow as only Oracle cares about this, and the Token could be used across Postgres and Oracle (and maybe Cassandra?)

gsheasby · 2017-10-17T12:55:30Z

...n/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/sweep/DbKvsGetCandidateCellsForSweeping.java


 public class DbKvsGetCandidateCellsForSweeping {

-    private final CellTsPairLoaderFactory cellTsPairLoaderFactory;
+    private final CellTsPairLoader cellTsPairLoaderFactory;


Found another non-rename. Intellij lets you do the rename automagically.

gsheasby · 2017-10-17T12:57:53Z

...n/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/sweep/DbKvsGetCandidateCellsForSweeping.java

-        return Iterators.filter(new PageIterator(loader, state), page -> !page.isEmpty());
+        Iterator<List<CellTsPairInfo>> cellTsIter = cellTsPairLoaderFactory.createPageIterator(tableRef, request);
+        Iterator<List<CandidateCellForSweeping>> rawIter = CandidateGroupingIterator.create(cellTsIter);
+        return Iterators.filter(rawIter, page -> !page.isEmpty());


Does this Iterator do an eager page load of the kind we were trying to avoid in OracleCellTsPageLoader.PageIterator?

gsheasby · 2017-10-17T13:03:51Z

Thanks for following through quickly with the refactor!

having-next-ness is a state, not an argument

Well... two, actually. This one is a CellTsPairToken. We already had a Token class in DbKvs :-/

I don't think we can do it nicely

gsheasby

I made the changes I want to make - deferring review of this to @hsaraogi.

There's probably more we can do here (the two PageIterator classes are rather similar), but I don't think it's worth it at this stage.

hsaraogi · 2017-10-17T17:06:28Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/sweep/CellTsPairToken.java, line 23 at r2 (raw file):

import com.palantir.atlasdb.encoding.PtBytes;

public final class CellTsPairToken {

Value.Immutable?

Comments from Reviewable

hsaraogi · 2017-10-17T17:07:28Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/sweep/CellTsPairToken.java, line 23 at r2 (raw file):

Previously, hsaraogi (Himangi Saraogi) wrote…

Value.Immutable?

We'll have a builder that will make the various types of tokens easier to construct..

Comments from Reviewable

hsaraogi · 2017-10-17T17:08:38Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleCellTsPageLoader.java, line 116 at r2 (raw file):

        final int sqlRowLimit;

        CellTsPairToken token;

final?

Comments from Reviewable

hsaraogi · 2017-10-17T17:14:05Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleCellTsPageLoader.java, line 230 at r1 (raw file):

Previously, gsheasby (Glenn Sheasby) wrote…

We're iterating through the results twice - once to bundle each one into a CellTsPairInfo, and again to figure out the count of cellTsPairsAlreadyExaminedInCurrentRow.

Is it worth cutting this iteration time in half by updating cellTsPairsAlreadyExaminedInCurrentRow within loadPage()?

Discussed offline with Glenn, doesnt seem like this significantly effects the time complexity.

Comments from Reviewable

hsaraogi · 2017-10-17T17:15:49Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleCellTsPageLoader.java, line 258 at r2 (raw file):

            } else {
                CellTsPairInfo lastResult = Iterables.getLast(results);
                Preconditions.checkState(lastResult.ts != Long.MAX_VALUE);

Lets add a message for the IllegalStateException here.

Comments from Reviewable

hsaraogi · 2017-10-17T17:19:38Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleCellTsPageLoader.java, line 258 at r2 (raw file):

Previously, hsaraogi (Himangi Saraogi) wrote…

Lets add a message for the IllegalStateException here.

Out of curiosity why do we need this check?

Comments from Reviewable

hsaraogi · 2017-10-17T17:20:24Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleCellTsPageLoader.java, line 258 at r2 (raw file):

Previously, hsaraogi (Himangi Saraogi) wrote…

Out of curiosity why do we need this check?

Will be clearer to have the precondition in continueRow

Comments from Reviewable

hsaraogi · 2017-10-17T17:24:36Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/postgres/PostgresCellTsPageLoader.java, line 95 at r2 (raw file):

        @Override
        public List<CellTsPairInfo> next() {
            Preconditions.checkState(hasNext());

This seems strange, an iterator checking hasNext in next and throwing?

Comments from Reviewable

hsaraogi · 2017-10-17T17:25:32Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/postgres/PostgresCellTsPageLoader.java, line 95 at r2 (raw file):

Previously, hsaraogi (Himangi Saraogi) wrote…

This seems strange, an iterator checking hasNext in next and throwing?

We should either not throw here and return an empty list or just throw the exception (if any) we get when trying to load a page.

Comments from Reviewable

hsaraogi · 2017-10-17T17:29:43Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/postgres/PostgresCellTsPageLoader.java, line 128 at r2 (raw file):

        private AgnosticLightResultSet selectNextPage(ConnectionSupplier conns) {
            FullQuery query = getQuery();

query -> fullQuery.

Comments from Reviewable

hsaraogi · 2017-10-17T17:29:48Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/postgres/PostgresCellTsPageLoader.java, line 132 at r2 (raw file):

        }

        private FullQuery getQuery() {

Rename to getFullQuery. or we have getQuery().getQuery().

Comments from Reviewable

hsaraogi · 2017-10-17T17:30:30Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/postgres/PostgresCellTsPageLoader.java, line 182 at r2 (raw file):

            } else {
                CellTsPairInfo lastResult = Iterables.getLast(results);
                Preconditions.checkState(lastResult.ts != Long.MAX_VALUE);

Lets move the check to continueRow.

Comments from Reviewable

hsaraogi · 2017-10-17T17:30:45Z

Reviewed 4 of 15 files at r1, 3 of 5 files at r2.
Review status: 5 of 13 files reviewed at latest revision, 12 unresolved discussions.

Comments from Reviewable

gbonik · 2017-10-17T17:54:30Z

Review status: 4 of 13 files reviewed at latest revision, 12 unresolved discussions.

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/postgres/PostgresCellTsPageLoader.java, line 95 at r2 (raw file):

Previously, hsaraogi (Himangi Saraogi) wrote…

We should either not throw here and return an empty list or just throw the exception (if any) we get when trying to load a page.

Why is it strange? This is how iterators are supposed to work. Try this: Collections.emptyList().iterator().next();

The only strange this about this is that a more common exception type to throw is NoSuchElementException - I am being lazy here and using Preconditions.checkState instead

Comments from Reviewable

* Move duplicated precondition to CellTsPairToken and add error message * Renames

gbonik · 2017-10-17T18:02:19Z

Review status: 4 of 13 files reviewed at latest revision, 12 unresolved discussions.

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/sweep/DbKvsGetCandidateCellsForSweeping.java, line 40 at r1 (raw file):

Previously, gsheasby (Glenn Sheasby) wrote…

Does this Iterator do an eager page load of the kind we were trying to avoid in OracleCellTsPageLoader.PageIterator?

Yes, the filtering iterator indeed needs to load the next page when hasNext() is called. However, this is not a problem: every call to hasNext() will be followed by next(), so no work is wasted. The reason why I was trying to avoid that in PageIterators is that CandidateGroupingIterator calls PageIterator.hasNext() after each call to PageIterator.next().

Comments from Reviewable

gsheasby · 2017-10-17T18:05:22Z

Review status: 3 of 13 files reviewed at latest revision, 11 unresolved discussions.

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleCellTsPageLoader.java, line 116 at r2 (raw file):

Previously, hsaraogi (Himangi Saraogi) wrote…

final?

No - it's updated by this class. This variable now encapsulates all the state.

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleCellTsPageLoader.java, line 258 at r2 (raw file):

Previously, hsaraogi (Himangi Saraogi) wrote…

Will be clearer to have the precondition in continueRow

Done

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/postgres/PostgresCellTsPageLoader.java, line 128 at r2 (raw file):

Previously, hsaraogi (Himangi Saraogi) wrote…

query -> fullQuery.

Done

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/postgres/PostgresCellTsPageLoader.java, line 132 at r2 (raw file):

Previously, hsaraogi (Himangi Saraogi) wrote…

Rename to getFullQuery. or we have getQuery().getQuery().

Done. FullQuery isn't a perfect name (neither is getQuery(), but that's beyond the scope of this PR)

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/sweep/CellTsPairToken.java, line 23 at r2 (raw file):

Previously, hsaraogi (Himangi Saraogi) wrote…

We'll have a builder that will make the various types of tokens easier to construct..

Done

Comments from Reviewable

gbonik · 2017-10-17T18:05:51Z

Review status: 3 of 13 files reviewed at latest revision, 11 unresolved discussions.

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/oracle/OracleCellTsPageLoader.java, line 258 at r2 (raw file):

Out of curiosity why do we need this check?

We need this check because we increment the timestamp by one to find the next starting position. So if it is equal to Long.MAX_VALUE, it would overflow. This should never be the case, but we check anyway for paranoia reasons

Comments from Reviewable

gsheasby · 2017-10-17T18:07:45Z

Review status: 3 of 13 files reviewed at latest revision, 8 unresolved discussions.

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/postgres/PostgresCellTsPageLoader.java, line 182 at r2 (raw file):

Previously, hsaraogi (Himangi Saraogi) wrote…

Lets move the check to continueRow.

Done

Comments from Reviewable

hsaraogi · 2017-10-17T18:47:29Z

atlasdb-dbkvs/src/test/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/sweep/CandidateGroupingIteratorTest.java, line 104 at r3 (raw file):

                .isLatestValueEmpty(latestValEmpty)
                .numCellsTsPairsExamined(numCellTsPairsExamined)
                .sortedTimestamps(ts)

Should be verify that ts is sorted here?

Comments from Reviewable

hsaraogi · 2017-10-17T18:48:52Z

Some very small comments, looks good overall.

Comments from Reviewable

hsaraogi · 2017-10-17T18:49:05Z

Reviewed 9 of 15 files at r1, 2 of 5 files at r2, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

gbonik requested a review from dxiao October 17, 2017 02:47

tboam assigned gsheasby Oct 17, 2017

tboam requested a review from gsheasby October 17, 2017 10:41

gsheasby added the component: sweep label Oct 17, 2017

gsheasby reviewed Oct 17, 2017

View reviewed changes

gsheasby added 9 commits October 17, 2017 16:13

Renames

5d2d93d

Create OracleCellTsPageLoader.PageIterator.Token class

9ea48bb

Create PostgresCellTsPageLaoder.PageIterator.Token

5c7c0e3

Fix Precondition

cbbfff0

having-next-ness is a state, not an argument

One Token To Rull Them All

e6aa802

Well... two, actually. This one is a CellTsPairToken. We already had a Token class in DbKvs :-/

Make CellTsPairToken final; replace modification with factory methods

f5de0c2

Have computeNextStartPosition return a token

33627f9

Remove the TODO

05ff84b

I don't think we can do it nicely

empty commit

6c3c05a

gsheasby approved these changes Oct 17, 2017

View reviewed changes

gsheasby assigned hsaraogi and unassigned gsheasby Oct 17, 2017

gsheasby requested a review from hsaraogi October 17, 2017 16:21

gsheasby added 2 commits October 17, 2017 18:55

Nits

117277a

* Move duplicated precondition to CellTsPairToken and add error message * Renames

Make CellTsPairToken immutable

751f3ae

hsaraogi approved these changes Oct 17, 2017

View reviewed changes

dxiao approved these changes Oct 18, 2017

View reviewed changes

Sort timestamps array, just in case

34dfa39

gsheasby merged commit 53b8cbf into develop Oct 18, 2017

gsheasby deleted the feature/sweep-paging-refactor branch October 18, 2017 11:58

This was referenced Aug 30, 2022

Bump postgres pgjdbc driver to 42.5.0 (fix CVE-2022-31197) #6206

Closed

Bump PostgreSQL driver from 42.3.3 -> 42.4.1 to remediate CVE-2022-31197 #6205

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DbKvs sweep refactors to address last PR comments #2497

DbKvs sweep refactors to address last PR comments #2497

gbonik commented Oct 17, 2017 •

edited by jboreiko

Loading

codecov-io commented Oct 17, 2017 •

edited

Loading

gsheasby left a comment

gsheasby Oct 17, 2017

gsheasby Oct 17, 2017

gsheasby Oct 17, 2017

gsheasby Oct 17, 2017

gsheasby Oct 17, 2017

gsheasby Oct 17, 2017

gsheasby commented Oct 17, 2017

gsheasby left a comment

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

gbonik commented Oct 17, 2017

gbonik commented Oct 17, 2017

gsheasby commented Oct 17, 2017

gbonik commented Oct 17, 2017

gsheasby commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

DbKvs sweep refactors to address last PR comments #2497

DbKvs sweep refactors to address last PR comments #2497

Conversation

gbonik commented Oct 17, 2017 • edited by jboreiko Loading

codecov-io commented Oct 17, 2017 • edited Loading

Codecov Report

gsheasby left a comment

Choose a reason for hiding this comment

gsheasby Oct 17, 2017

Choose a reason for hiding this comment

gsheasby Oct 17, 2017

Choose a reason for hiding this comment

gsheasby Oct 17, 2017

Choose a reason for hiding this comment

gsheasby Oct 17, 2017

Choose a reason for hiding this comment

gsheasby Oct 17, 2017

Choose a reason for hiding this comment

gsheasby Oct 17, 2017

Choose a reason for hiding this comment

gsheasby commented Oct 17, 2017

gsheasby left a comment

Choose a reason for hiding this comment

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

gbonik commented Oct 17, 2017

gbonik commented Oct 17, 2017

gsheasby commented Oct 17, 2017

gbonik commented Oct 17, 2017

gsheasby commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

hsaraogi commented Oct 17, 2017

gbonik commented Oct 17, 2017 •

edited by jboreiko

Loading

codecov-io commented Oct 17, 2017 •

edited

Loading