[Timelock Corruption]: Final Wiring part 1 #5071

sudiksha27 · 2020-10-22T09:20:04Z

Goals (and why):
Wire corruption components

Implementation Description (bullets):

Basically connects all pieces together

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

Concerns (what feedback would you like?):

Is it correct to initialize the PaxosHistoryProvider in PaxosResourceFactory?
Did I miss something?

Where should we start reviewing?:

PaxosResourceFactory

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

Jolyon-S

Just picked up some nits in a drive-by.

Jolyon-S · 2020-10-22T15:11:13Z

...etection/src/main/java/com/palantir/timelock/corruption/detection/CorruptionHealthCheck.java

 import java.util.List;

 public class CorruptionHealthCheck {
-    private final List<CorruptionDetector> corruptionDetectors;
+    private final LocalCorruptionDetector localCorruptionDetector;
+    private final List<CorruptionDetector> localAndRemoteCorruptionDetectors;


this feels a bit weird (although maybe it's an Atlas thing?) - that is, using a List to represent two and exactly two distinct things here. I'd prefer if you just made a small Immutables to hold the local and remote, then you only need one field and can call something like corruptionDetectors.local() (although naturally this doesn't lend itself trivially to your streams).

Have a look at LocalAndRemotes? It's a bit more powerful than what you need here, but has a nice way of accessing all or specifically the local.

Jolyon-S · 2020-10-22T15:12:18Z

...ection/src/main/java/com/palantir/timelock/corruption/detection/LocalCorruptionDetector.java

@@ -31,46 +32,63 @@

    private final ScheduledExecutorService executor = PTExecutors.newSingleThreadScheduledExecutor(
            new NamedThreadFactory(CORRUPTION_DETECTOR_THREAD_PREFIX, true));
+


nit: unnecessary diff.

Jolyon-S · 2020-10-22T15:13:00Z

...-detection/src/main/java/com/palantir/timelock/corruption/handle/JerseyCorruptionFilter.java

@@ -32,7 +32,7 @@ public JerseyCorruptionFilter(CorruptionHealthCheck healthCheck) {

    @Override
    public void filter(ContainerRequestContext requestContext) {
-        if (!healthCheck.isHealthy()) {
+        if (healthCheck.shootTimeLock()) {


I'm not really a fan of shootTimelock - it feels pretty ambiguous (particularly as this is a boolean!)

jeremyk-91

Looks correct. Main things:

shootTimeLock() looks like a command, I'd suggest making it clearer that it is a predicate (e.g. shouldRejectUserRequests())
I don't really like the findFirst() when identifying rogue clients, it seems we should just report all failing checks when multiple are unhappy.

jeremyk-91 · 2020-10-22T16:10:23Z

...n-detection/src/main/java/com/palantir/timelock/corruption/detection/CorruptionDetector.java

@@ -17,5 +17,5 @@
 package com.palantir.timelock.corruption.detection;

 public interface CorruptionDetector {
-    boolean hasDetectedCorruption();
+    boolean shootTimeLock();


maybe shouldRejectRequests()? The impression I get if I come across a method like this is that it actually makes the server stop requests and returns true if successful/false if not

jeremyk-91 · 2020-10-22T16:12:27Z

...etection/src/main/java/com/palantir/timelock/corruption/detection/CorruptionHealthCheck.java

 import java.util.List;

 public class CorruptionHealthCheck {
-    private final List<CorruptionDetector> corruptionDetectors;
+    private final LocalCorruptionDetector localCorruptionDetector;
+    private final List<CorruptionDetector> localAndRemoteCorruptionDetectors;


Have a look at LocalAndRemotes? It's a bit more powerful than what you need here, but has a nice way of accessing all or specifically the local.

jeremyk-91 · 2020-10-22T16:16:31Z

...tion-detection/src/main/java/com/palantir/timelock/corruption/detection/HistoryAnalyzer.java

+                                HistoryAnalyzer::extractNamespaceAndUseCase,
+                                HistoryAnalyzer::violatedCorruptionChecksForNamespaceAndUseCase));
+
+        SetMultimap<CorruptionCheckViolation, NamespaceAndUseCase> entryEntrySetMultimap = KeyedStream.stream(


Let's name this based on what the map actually does: namespacesExhibitingViolations or something like this.

jeremyk-91 · 2020-10-22T16:21:58Z

...tion-detection/src/main/java/com/palantir/timelock/corruption/detection/HistoryAnalyzer.java

    @VisibleForTesting
-    static List<CorruptionCheckViolation> violatedCorruptionChecksForNamespaceAndUseCase(
+    static CorruptionCheckViolation violatedCorruptionChecksForNamespaceAndUseCase(


I think this should return List or some Collection given the name?

Right, the issue is that (and I missed this while writing #5057) these checks are to be executed in an order, e.g. the check for acceptor quorum for a learned value does not hold value if the learners have diverged as we anyway do not have a learner value to compare accepted values against.

jeremyk-91 · 2020-10-22T16:22:44Z

...tion-detection/src/main/java/com/palantir/timelock/corruption/detection/HistoryAnalyzer.java

+        Function<CompletePaxosHistoryForNamespaceAndUseCase, CorruptionCheckViolation> divergedLearnedCheck =
+                HistoryAnalyzer::divergedLearners;
+        Function<CompletePaxosHistoryForNamespaceAndUseCase, CorruptionCheckViolation> learnedValueWithoutQuorum =
+                HistoryAnalyzer::learnedValueWithoutQuorum;
+        Function<CompletePaxosHistoryForNamespaceAndUseCase, CorruptionCheckViolation> greatestAcceptedValueNotLearned =
+                HistoryAnalyzer::greatestAcceptedValueNotLearned;
+
+        return Stream.of(divergedLearnedCheck, learnedValueWithoutQuorum, greatestAcceptedValueNotLearned)


Might be worth extracting a constant for the set of checks

jeremyk-91 · 2020-10-22T16:28:17Z

...ection/src/main/java/com/palantir/timelock/corruption/detection/LocalCorruptionDetector.java

+
+    private void processLocalHealthReport() {
+        localCorruptionState = getLocalCorruptionState(localCorruptionReport);
+        if (localCorruptionState.shootTimeLock()) {


as flagged elsewhere, this gets confusing, I think because it looks like a command, but it's not

jeremyk-91 · 2020-10-22T16:30:58Z

...-detection/src/test/java/com/palantir/timelock/corruption/detection/HistoryAnalyzerTest.java

+        assertThat(HistoryAnalyzer.corruptionHealthReportForHistory(historyForAll)
+                        .statusesToNamespaceAndUseCase()
+                        .size())
+                .isEqualTo(0);


nit: Prefer isEmpty()

jeremyk-91 · 2020-10-22T16:31:27Z

...-detection/src/test/java/com/palantir/timelock/corruption/detection/HistoryAnalyzerTest.java

+                .isEqualTo(CorruptionCheckViolation.NONE);
+
+        assertThat(HistoryAnalyzer.corruptionHealthReportForHistory(historyForAll)
+                        .statusesToNamespaceAndUseCase()


could we rename this violatingStatuses... or something like that?

jeremyk-91

discussed: yeah ordered checks make a lot of sense!

svc-autorelease · 2020-10-23T11:37:22Z

Released 0.257.0

sudiksha27 added 4 commits October 22, 2020 10:10

Wire through - part 1

2e08991

Refactor

d5594fe

Fix + tests

9bd0d73

Refactor

ddd99f2

sudiksha27 added the no changelog label Oct 22, 2020

sudiksha27 marked this pull request as ready for review October 22, 2020 15:00

sudiksha27 changed the title ~~Wire through - part 1~~ [Timelock Corruption]: Wire through Oct 22, 2020

sudiksha27 changed the title ~~[Timelock Corruption]: Wire through~~ [Timelock Corruption]: Final Wiring part 1 Oct 22, 2020

Jolyon-S reviewed Oct 22, 2020

View reviewed changes

jeremyk-91 reviewed Oct 22, 2020

View reviewed changes

Address comments - part 1

bd02856

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

Fix build

4c6c757

jeremyk-91 approved these changes Oct 23, 2020

View reviewed changes

Refactor

bbe834d

sudiksha27 added autorelease and removed pr: needs revision labels Oct 23, 2020

sudiksha27 merged commit ce7ec74 into develop Oct 23, 2020

delete-merged-branch bot deleted the wire-ete-2 branch October 23, 2020 11:37

sudiksha27 mentioned this pull request Oct 27, 2020

[TimeLock Corruption] | Implement progress tracker #5078

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Timelock Corruption]: Final Wiring part 1 #5071

[Timelock Corruption]: Final Wiring part 1 #5071

sudiksha27 commented Oct 22, 2020 •

edited

Loading

Jolyon-S left a comment

Jolyon-S Oct 22, 2020

jeremyk-91 Oct 22, 2020

Jolyon-S Oct 22, 2020

Jolyon-S Oct 22, 2020

jeremyk-91 left a comment

jeremyk-91 Oct 22, 2020

jeremyk-91 Oct 22, 2020

jeremyk-91 Oct 22, 2020

jeremyk-91 Oct 22, 2020

sudiksha27 Oct 22, 2020

jeremyk-91 Oct 22, 2020

jeremyk-91 Oct 22, 2020

jeremyk-91 Oct 22, 2020

jeremyk-91 Oct 22, 2020

jeremyk-91 left a comment

svc-autorelease commented Oct 23, 2020

		@@ -31,46 +32,63 @@

		private final ScheduledExecutorService executor = PTExecutors.newSingleThreadScheduledExecutor(
		new NamedThreadFactory(CORRUPTION_DETECTOR_THREAD_PREFIX, true));

[Timelock Corruption]: Final Wiring part 1 #5071

[Timelock Corruption]: Final Wiring part 1 #5071

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

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

jeremyk-91 left a comment

Choose a reason for hiding this comment

svc-autorelease commented Oct 23, 2020

sudiksha27 commented Oct 22, 2020 •

edited

Loading