[TimeLock Corruption] | Implement progress tracker #5078

sudiksha27 · 2020-10-22T17:14:09Z

Goals (and why):
Adds a ProgressTracker to keep track of last verified sequence numbers + update last verified one history till that point is loaded from all servers.

Implementation Description (bullets):

Add a ProgressTracker to maintain local cache, get lastVerified seq numbers and update lastVerified
Maintain local cache of (lastVerifiedSeq, greatestSeqNumberToBeVerified) for each (namespace, useCase) pair
Init state => lastVerifiedSeq = -1, greatestSeqNumberToBeVerified = latest learned value for respective (namespace, useCase) pair
Each iteration will do two things -
- get seq bounds [lowerInclusive, upperInclusive] to load logs for corruption verification. If the lastVerifiedSeq > greatestSeqNumberToBeVerified, reset to init state
- update the lastVerified with upperInclusive after logs have been loaded

Testing (What was existing testing like? What have you done to improve it?):
Added tests

Concerns (what feedback would you like?):
Did I miss any edge cases in the ProgressTracker?

No batching while making DB calls
No mechanism for detecting inactive series. In the worst case, inactive series can get their progressState refreshed on each iteration.

Where should we start reviewing?:
PaxosLogHistoryProgressTracker.java

Priority (whenever / two weeks / yesterday):
ASAP 🏃‍♀️

Jolyon-S

Drive-by of a few things; didn't review in-depth semantics as I don't have full context on what's going on here.

Jolyon-S · 2020-10-23T14:56:11Z

...on-detection/src/main/java/com/palantir/timelock/history/PaxosLogHistoryProgressTracker.java

+import java.util.concurrent.ConcurrentHashMap;
+import javax.sql.DataSource;
+
+public class PaxosLogHistoryProgressTracker {


IntelliJ is telling me many things in this class don't have to be public - can you make those package-private or private?

Jolyon-S · 2020-10-23T14:56:24Z

...on-detection/src/main/java/com/palantir/timelock/history/PaxosLogHistoryProgressTracker.java

+
+    public void updateProgressState(Map<NamespaceAndUseCase, SequenceBounds> namespaceAndUseCaseSequenceBoundsMap) {
+        namespaceAndUseCaseSequenceBoundsMap.forEach((namespaceAndUseCase, bounds) ->
+                updateProgressStateForNamespaceAndUseCase(namespaceAndUseCase, bounds));


replace with method reference

Jolyon-S · 2020-10-23T15:00:16Z

...on-detection/src/main/java/com/palantir/timelock/history/PaxosLogHistoryProgressTracker.java

+    private final LogVerificationProgressState logVerificationProgressState;
+    private final SqlitePaxosStateLogHistory sqlitePaxosStateLogHistory;
+
+    private Map<NamespaceAndUseCase, ProgressComponents> verificationProgressStateCache = new ConcurrentHashMap<>();


As far as I can tell, you only ever put or get from this cache (i.e. never delete). I don't have full context on this piece of work, but I wonder if it is possible to OOM yourself? How many entries do you expect to be present in this cache?

The size would be bounded by |NamespaceAndUseCase| which would generally be around 1000, so I think this is fine. (Good flag though!)

Jolyon-S · 2020-10-23T15:02:08Z

...ruption-detection/src/main/java/com/palantir/timelock/history/models/ProgressComponents.java

+import org.immutables.value.Value;
+
+@Value.Immutable
+public interface ProgressComponents {


I think these immutables are fine, but if you really want to avoid crossed wires, you can try a staged builder: https://immutables.github.io/immutable.html#staged-builder

(I appreciate not everyone likes them, but I do!)

Jolyon-S · 2020-10-23T15:06:38Z

...etection/src/main/java/com/palantir/timelock/history/remote/HistoryLoaderAndTransformer.java

-        Map<NamespaceAndUseCase, Long> lastVerifiedSequences = historyQueries.stream()
-                .collect(Collectors.toMap(HistoryQuery::getNamespaceAndUseCase, HistoryQuery::getSeq, Math::min));
+        Map<NamespaceAndUseCase, SequenceBounds> lastVerifiedSequences = historyQueries.stream()
+                .collect(Collectors.toMap(


I'm not a huge fan - KeyedStream.of(historyQueries).mapKeys(...).map(...) gets you most of the way, although I'll confess that I'm not sure how to do the merging resolution in KeyedStream (although you can always collect to multimap).

I think what we have now is more readable

Jolyon-S · 2020-10-23T15:08:33Z

...n-detection/src/test/java/com/palantir/timelock/history/HistoryLoaderAndTransformerTest.java

        List<LogsForNamespaceAndUseCase> paxosHistory =
                HistoryLoaderAndTransformer.getLogsForHistoryQueries(history, historyQueries);

-        sanityCheckLoadedHistory(paxosHistory, 100 - lastVerified);
+        sanityCheckLoadedHistory(paxosHistory, 100 - lastVerified + 1);


Speaking of sanity checks, is this:

(100 - lastVerified) + 1
OR
100 - (lastVerified + 1)?

I assume the latter, although I'm never certain on how it determines this - so maybe bracket it?

jeremyk-91

The structure makes sense, but I have some concerns around readability. Specifically, you use lastVerifiedSequence throughout most of the code, but associate it with an exclusive upper bound in SequenceBounds which means that the "last verified sequence" isn't actually verified. I'd suggest using inclusive bounds throughout.

I think this also leads to a strange edge case in PaxosLogHistoryProgressTracker: as written I think the tracker spins infinitely on an inactive series, because you're comparing an inclusive bound with an exclusive bound.

jeremyk-91 · 2020-10-23T14:29:43Z

...ruption-detection/src/main/java/com/palantir/timelock/history/models/ProgressComponents.java

+    long seq();
+
+    @Value.Parameter
+    long progressLimit();


nit: could we have docs on what this means?

jeremyk-91 · 2020-10-23T15:23:41Z

...etection/src/main/java/com/palantir/timelock/history/remote/HistoryLoaderAndTransformer.java

@@ -47,6 +52,17 @@ private HistoryLoaderAndTransformer() {
                .collect(Collectors.toList());
    }

+    private static SequenceBounds seqBoundsCollisionResolver(SequenceBounds bound1, SequenceBounds bound2) {


nit: let's give this a more descriptive name? minimalLowerBoundResolver or something like that?

jeremyk-91 · 2020-10-23T15:27:56Z

...n-detection/src/test/java/com/palantir/timelock/history/HistoryLoaderAndTransformerTest.java

@@ -87,13 +88,13 @@ public void canHandleDuplicateQueries() {

        List<HistoryQuery> queries = IntStream.range(0, 10)
                .boxed()
-                .map(idx -> HistoryQuery.of(NAMESPACE_AND_USE_CASE, minLastVerified + idx))
+                .map(idx -> HistoryQuery.of(NAMESPACE_AND_USE_CASE, minLastVerified + idx, minLastVerified + 500))


can we reference this 500 constant somewhere?

actually, see later comment: would prefer this to have a factory for generating unbounded ends

jeremyk-91 · 2020-10-23T15:31:16Z

...ption-detection/src/test/java/com/palantir/timelock/history/PaxosLogHistoryProviderTest.java

-        List<HistoryQuery> historyQueries =
-                ImmutableList.of(HistoryQuery.of(ImmutableNamespaceAndUseCase.of(CLIENT, USE_CASE), lastVerified));
+        List<HistoryQuery> historyQueries = ImmutableList.of(
+                HistoryQuery.of(ImmutableNamespaceAndUseCase.of(CLIENT, USE_CASE), lastVerified, lastVerified + 500));


Maybe just have an unbounded end factory in HistoryQuery, it looks like this is causing a lot of friction

jeremyk-91 · 2020-10-23T15:34:44Z

...-corruption-detection/src/main/java/com/palantir/timelock/history/models/SequenceBounds.java

+    long lower();
+
+    @Value.Parameter
+    long upper();


I'd recommend being explicit about the edges of the range here: lowerInclusive but upperExclusive?

Actually, I would strongly prefer for us to use inclusive ranges. A lot of the lastVerified stuff is misleading because upper() is not actually verified.

jeremyk-91 · 2020-10-23T16:57:29Z

...ruption-detection/src/main/java/com/palantir/timelock/history/models/ProgressComponents.java

+import org.immutables.value.Value;
+
+@Value.Immutable
+public interface ProgressComponents {


I'd name this differently, maybe ProgressState or something like that?

jeremyk-91 · 2020-10-23T16:57:55Z

...ruption-detection/src/main/java/com/palantir/timelock/history/models/ProgressComponents.java

+@Value.Immutable
+public interface ProgressComponents {
+    @Value.Parameter
+    long seq();


lastVerifiedSeq? This makes it sound similar to the Paxos constructs when it's really not

jeremyk-91 · 2020-10-23T17:07:52Z

...detection/src/main/java/com/palantir/timelock/history/sqlite/SqlitePaxosStateLogHistory.java

-        Map<Long, PaxosAcceptorData> getAcceptorLogsSince(
-                @BindPojo("namespace") Client namespace, @Bind("useCase") String useCase, @Bind("seq") long seq);
+        @SqlQuery("SELECT seq, val FROM paxosLog WHERE namespace = :namespace.value AND useCase = :useCase AND seq >="
+                + " :lowerBound AND seq < :upperBound")


See discussion elsewhere: I think I'd have a strong preference for having inclusive ranges here, because we've used terminology like latestVerified through many other parts of the code, which are not strictly correct when the upper bound has actually not been verified.

jeremyk-91 · 2020-10-23T17:18:21Z

...on-detection/src/main/java/com/palantir/timelock/history/PaxosLogHistoryProgressTracker.java

+
+    @VisibleForTesting
+    void updateProgressStateForNamespaceAndUseCase(NamespaceAndUseCase key, SequenceBounds value) {
+        long lastVerifiedSequence = value.upper();


This is an exclusive bound, so it is not the last verified sequence (see comment on my thoughts on SequenceBound).

jeremyk-91 · 2020-10-23T17:23:58Z

...on-detection/src/main/java/com/palantir/timelock/history/PaxosLogHistoryProgressTracker.java

+            NamespaceAndUseCase key, SequenceBounds value, ProgressComponents currentProgressState) {
+        if (value.upper() <= currentProgressState.progressLimit()) {
+            return Optional.empty();
+        }


This doesn't behave the way I think is intended: your seeding of the bound in getLatestSequence is inclusive, however your use of value here is exclusive. This means that even if there are no new entries, the validation task will repeatedly cycle over the entries (e.g. of an inactive client), which I doubt is intended.

jeremyk-91 · 2020-10-23T17:29:52Z

I'd suggest having as integration tests (maybe not in this PR, but before the workstream is complete):

verifying a sequence with over 500 entries (so that it needs to be verified in batches) works correctly
an invalid sequence at entry 499 and 500 are caught
inactive sequence does not get verified more than once.

sudiksha27

I agree, I'll add some ETE tests in a separate PR.

sudiksha27 · 2020-10-26T13:15:37Z

...etection/src/main/java/com/palantir/timelock/history/remote/HistoryLoaderAndTransformer.java

-        Map<NamespaceAndUseCase, Long> lastVerifiedSequences = historyQueries.stream()
-                .collect(Collectors.toMap(HistoryQuery::getNamespaceAndUseCase, HistoryQuery::getSeq, Math::min));
+        Map<NamespaceAndUseCase, SequenceBounds> lastVerifiedSequences = historyQueries.stream()
+                .collect(Collectors.toMap(


I think what we have now is more readable

jeremyk-91

🕐 1:00

Thanks for adding more comprehensive tests and changing the interval queries to operate on closed intervals; I think the PR looks correct now.

I'm afraid I think there are still some issues, largely around readability:

I'm uncertain of the decomp: I don't understand the point of SequenceBounds when HistoryQuery already exists. I'd kill it if possible, and if not, given that you have a constant in the class and a factory that strongly suggest it is intended only for history queries, I would suggest naming it accordingly.
Some variable names, in particular around the form of Map<T, SequenceBounds> lastVerifiedSequences need to be changed - in particular, they often refer to the bounds on queries that need to be made (while previously there was only a lower bound, so lastVerifiedSequences was correct).
I would urge you to use method names indicating the intention behind your methods as opposed to just what the methods do. For example, getBoundsSinceLastVerified(long lastVerified) doesn't say much, and I'd argue the most natural interpretation of that is [lastVerified + 1, inf) - while if you describe it as getBoundsForNextHistoryQuery(long lastVerified) a reader will understand what it's for, and is more likely to have assumptions in line with what you're intending.

jeremyk-91 · 2020-10-26T20:31:04Z

...tection/src/main/java/com/palantir/timelock/history/sqlite/LogVerificationProgressState.java

-        @SqlUpdate("CREATE TABLE IF NOT EXISTS logVerificationProgress (namespace TEXT, useCase TEXT, seq BIGINT,"
-                + "PRIMARY KEY(namespace, useCase))")
+        @SqlUpdate("CREATE TABLE IF NOT EXISTS logVerificationProgress (namespace TEXT, useCase TEXT, lastVerifiedSeq"
+                + " BIGINT, greatestSeqNumberToBeVerified BIGINT, PRIMARY KEY(namespace, useCase))")


sanity check: this has never been wired up right?

This will break since #5071 merged, my bad, I will fix this.

jeremyk-91 · 2020-10-26T20:40:07Z

...detection/src/main/java/com/palantir/timelock/history/sqlite/SqlitePaxosStateLogHistory.java

+            LearnerUseCase learnerUseCase,
+            AcceptorUseCase acceptorUseCase,
+            long lowerBound,
+            long upperBound) {


I'd suggest maintaining lowerBoundInclusive and upperBoundInclusive through the parameters in the call stack when making these queries

jeremyk-91 · 2020-10-26T20:50:27Z

...orruption-detection/src/main/java/com/palantir/timelock/history/PaxosLogHistoryProvider.java

    //     TODO(snanda): Refactor the two parts on translating PaxosHistoryOnRemote to
    //      CompletePaxosHistoryForNamespaceAndUseCase to a separate component
    public List<CompletePaxosHistoryForNamespaceAndUseCase> getHistory() {
-        Map<NamespaceAndUseCase, Long> lastVerifiedSequences = getNamespaceAndUseCaseToLastVerifiedSeqMap();
+        Map<NamespaceAndUseCase, SequenceBounds> lastVerifiedSequences = getNamespaceAndUseCaseToLastVerifiedSeqMap();


This variable name is misleading. These bounds are not verified at the time this call is made - they are the candidates you want to verify. In the previous model it was correct (because that's where you start the query from).

jeremyk-91 · 2020-10-26T20:54:12Z

...ock-corruption-detection/src/main/java/com/palantir/timelock/history/LocalHistoryLoader.java

@@ -39,19 +40,21 @@ public static LocalHistoryLoader create(SqlitePaxosStateLogHistory sqlitePaxosSt
        return new LocalHistoryLoader(sqlitePaxosStateLogHistory);
    }

-    public PaxosHistoryOnSingleNode getLocalPaxosHistory(Map<NamespaceAndUseCase, Long> lastVerifiedSequences) {
+    public PaxosHistoryOnSingleNode getLocalPaxosHistory(
+            Map<NamespaceAndUseCase, SequenceBounds> lastVerifiedSequences) {


The name is no longer correct.

jeremyk-91 · 2020-10-26T20:56:15Z

...orruption-detection/src/main/java/com/palantir/timelock/history/PaxosLogHistoryProvider.java

        return KeyedStream.stream(lastVerifiedSequences)
                .mapEntries(this::buildHistoryQuery)
                .values()
                .collect(Collectors.toList());
    }

-    private Map<NamespaceAndUseCase, Long> getNamespaceAndUseCaseToLastVerifiedSeqMap() {
+    private Map<NamespaceAndUseCase, SequenceBounds> getNamespaceAndUseCaseToLastVerifiedSeqMap() {


nit: The name of this method is no longer correct though. As a user who's not familiar, how do I know what the elements of SequenceBounds are?

jeremyk-91 · 2020-10-26T20:58:46Z

...-corruption-detection/src/main/java/com/palantir/timelock/history/models/SequenceBounds.java

+        return ImmutableSequenceBounds.builder();
+    }
+
+    static SequenceBounds getBoundsSinceLastVerified(long lastVerified) {


What does this mean?

I might be missing something, but I'm starting to be convinced that this class has no reason to exist: we should just operate on HistoryQuery objects.

jeremyk-91 · 2020-10-26T20:59:26Z

...on-detection/src/main/java/com/palantir/timelock/history/PaxosLogHistoryProgressTracker.java

+        this.sqlitePaxosStateLogHistory = sqlitePaxosStateLogHistory;
+    }
+
+    public SequenceBounds getPaxosLogSequenceBounds(NamespaceAndUseCase namespaceAndUseCase) {


What does this method name mean? (It gets the bounds for the next history query; is there a reason the progress tracker shouldn't be creating history queries in and of itself?)

jeremyk-91 · 2020-10-26T21:01:37Z

...-corruption-detection/src/main/java/com/palantir/timelock/history/models/SequenceBounds.java

+
+@Value.Immutable
+public interface SequenceBounds {
+    int MAX_ROWS_ALLOWED = 500;


It feels kind of dubious that this is here; it seems this is just a history query without a namespace and use case. If possible I'd urge you to try and kill this class altogether, but if that's not possible I would strongly prefer naming this class so that it's obvious that it is specific to history queries (e.g. HistoryQuerySequenceRange).

jeremyk-91 · 2020-10-26T21:02:09Z

...orruption-detection/src/test/java/com/palantir/timelock/history/utils/HistoryQueryUtils.java

+import com.palantir.paxos.NamespaceAndUseCase;
+import com.palantir.timelock.history.HistoryQuery;
+
+public class HistoryQueryUtils {


nit: the standard form for factory classes of this kind is the plural of the class name i.e. HistoryQueries

jeremyk-91

👍 Checkpointing of multiple values can be messy, but discussed offline - we think it's not unreasonable to remove it and use in memory state for remembering when we should reset.

There is a degenerate condition where you might never verify old entries if the server is restarted extremely frequently, so that there is always one paxos round in between the verification task reading the bound and processing a batch, but I don't think we need to worry about that.

changelog-app · 2020-10-27T16:26:07Z

Generate changelog in `changelog/@unreleased`

Type

Description

Adds a ProgressTracker to keep track of last verified sequence numbers for respective (namespace, useCase) pairs for TimeLock corruption checks. This ensures that TimeLock will resume corruption checks from after the lastVerifiedSequence if the node is bounced.

Check the box to generate changelog(s)

Generate changelog entry

delete-merged-branch bot deleted the branch develop October 23, 2020 11:37

sudiksha27 changed the base branch from wire-ete-2 to develop October 23, 2020 13:24

sudiksha27 marked this pull request as ready for review October 23, 2020 14:15

sudiksha27 requested review from jeremyk-91 and gmaretic October 23, 2020 14:16

sudiksha27 changed the title ~~WIP~~ [TimeLock Corruption] | Implement progress tracker Oct 23, 2020

Jolyon-S reviewed Oct 23, 2020

View reviewed changes

jeremyk-91 reviewed Oct 23, 2020

View reviewed changes

sudiksha27 added the pr: needs revision label Oct 26, 2020

sudiksha27 commented Oct 26, 2020

View reviewed changes

jeremyk-91 reviewed Oct 26, 2020

View reviewed changes

jeremyk-91 approved these changes Oct 27, 2020

View reviewed changes

sudiksha27 added no changelog and removed no changelog labels Oct 27, 2020

sudiksha27 added 14 commits October 27, 2020 20:46

Wire through - part 1

7023d19

WIP

190b0dc

Wire

7e3c8f3

WIP

551a265

Tests

ae2e8a8

Refactor

063a326

Remove redundant

a9a28c3

Major bux fix

d82ae18

Refactor - 1

35539e8

Modify algo - 1

4c33745

Unit tests

fbfe193

Address comments - 2

ced25a3

Test refactor

f751e77

Format

dbc6101

sudiksha27 added 10 commits October 27, 2020 20:56

Fix broken tests

0bd6e8e

Refactor

6369d99

Bug fix

31ee4ae

Small

64d39bf

Let's start reverting messy changes - 1

2b259f1

SequenceBounds -> HistoryQuerySequenceBounds

721f851

Name correction - 1

01963c9

Refactor

e61dd2a

Refactor

b4b291a

Add generated changelog entries

17ab35e

sudiksha27 force-pushed the corr-progress branch from 058038b to b4b291a Compare October 27, 2020 21:01

sudiksha27 added 3 commits October 28, 2020 09:18

Fix build

56b8050

Fix build - attempt II

b091297

This should not break my build

d42c4c3

sudiksha27 removed the pr: needs revision label Oct 28, 2020

sudiksha27 merged commit 905910a into develop Oct 28, 2020

delete-merged-branch bot deleted the corr-progress branch October 28, 2020 11:02

[TimeLock Corruption] | Implement progress tracker #5078

[TimeLock Corruption] | Implement progress tracker #5078

Conversation

sudiksha27 commented Oct 22, 2020 • edited Loading

Jolyon-S left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyk-91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyk-91 commented Oct 23, 2020

sudiksha27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyk-91 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyk-91 left a comment • edited Loading

Choose a reason for hiding this comment

changelog-app bot commented Oct 27, 2020 • edited by sudiksha27 Loading

Generate changelog in changelog/@unreleased

sudiksha27 commented Oct 22, 2020 •

edited

Loading

jeremyk-91 left a comment •

edited

Loading

jeremyk-91 left a comment •

edited

Loading

changelog-app bot commented Oct 27, 2020 •

edited by sudiksha27

Loading

Generate changelog in `changelog/@unreleased`