(CDAP-12646) fix run record fixer #9652

chtyim · 2017-09-21T10:10:23Z

Perform micro batches of scanning from store to avoid tx timeout
Removed all unnecessary point lookup of run records
- Make ProgramRunId available to RunRecordMeta
- Updated AppMetaStore deserialize RunRecordMeta with the ProgramRunId in it (which comes from the MDS key during scan, no extra lookup is needed).

anew

mostly minor comments from me.

I understand that the major change here is (apart from refactoring) that we now store the programRunId as part of the run record and therefore can avoid the point lookup for each record. Or did I miss anything?

anew · 2017-09-21T16:45:28Z

cdap-app-fabric/src/main/java/co/cask/cdap/internal/app/services/RunRecordCorrectorService.java

+    Set<ProgramRunId> fixed = doFixRunRecords();
+
+    if (!fixed.isEmpty()) {
+      LOG.info("Corrected {} of run records in {} status but no actual program running.",


Can you improve this message? (The params are reversed and the grammar is odd). Perhaps:

"Correct {n} run records with status in {statuses} that have no actual running program. Such programs likely have crashed or were killed by external signal. "

Sure. This was copied without much thought.

anew · 2017-09-21T16:48:40Z

cdap-app-fabric/src/main/java/co/cask/cdap/internal/app/services/RunRecordCorrectorService.java

+          startTime = Math.max(startTime, RunIds.getTime(record.getPid(), TimeUnit.SECONDS));
+
+          ProgramRunId programRunId = record.getProgramRunId();
+          if (!fixedPrograms.contains(programRunId)) {


isn't this already filtered by the createFilter(fixedProgram) above?

yeah, it is the same Set. Moving it outside the loop.

anew · 2017-09-21T16:51:17Z

cdap-app-fabric/src/main/java/co/cask/cdap/internal/app/services/RunRecordCorrectorService.java

+          if (!fixedPrograms.contains(programRunId)) {
+            programStateWriter.error(
+              programRunId,
+              new Throwable("Marking run record for " + programRunId + " as failed since no running program found."));


Can we have a specific exception class for this? Seems odd to use Throwable...

anew · 2017-09-21T16:54:09Z

cdap-app-fabric/src/main/java/co/cask/cdap/internal/app/services/RunRecordCorrectorService.java

+        }
+
+        // runs are marked as starting before they get into the runtimeService list.
+        // Therefore, add some buffer so that we don't incorrectly 'fix' runs that are actually starting


comment is confusing here, this is actually done at the end of the method. Maybe move it down there?

Haven't look at the message... it was copied.. I think it means don't fix record right away as the program running state may not be in the ProgramRuntimeService yet. I'll rephase the message better.

anew · 2017-09-21T16:55:52Z

cdap-app-fabric/src/main/java/co/cask/cdap/internal/app/store/AppMetadataStore.java

-    return GSON.fromJson(Bytes.toString(serialized), typeOfT);
+  protected <T> T deserialize(MDSKey key, byte[] serialized, Type typeOfT) {
+    if (RunRecordMeta.class.equals(typeOfT)) {
+      RunRecordMeta meta = GSON.fromJson(Bytes.toString(serialized), RunRecordMeta.class);


can you explain this? Does the Json not have the correct program/run ID?

The ProgramRunId is transient, hence is not serialized when stored into MDS (for backward compatibility and also not to duplicate what's in the row key already). Maybe we can reduce the temporary object creation by using a custom type adapter if necessary.

I think it's fine. If I had read this from bottom to top, I would have seen the changes down there first :)

anew · 2017-09-21T17:00:05Z

...p-fabric/src/test/java/co/cask/cdap/internal/app/services/RunRecordCorrectorServiceTest.java

+    }
+  }
+
+  private void validateExpectedState(ProgramRunId programRunId, ProgramRunStatus expectedStatus) {


this test case should be a lot faster now, right? 👍

yes and test more conditions.

chtyim · 2017-09-21T17:38:33Z

Thanks for the review @anew . Yes, the idea is to simply include the ProgramRunId in the RunRecordMeta during scan, such that we can remove the unnecessary expensive reserve lookup from RunId to ProgramId.

anew · 2017-09-21T18:15:39Z

cdap-app-fabric/src/main/java/co/cask/cdap/internal/app/services/RunRecordCorrectorService.java

@@ -76,8 +76,9 @@ void fixRunRecords() {
    Set<ProgramRunId> fixed = doFixRunRecords();

    if (!fixed.isEmpty()) {
-      LOG.info("Corrected {} of run records in {} status but no actual program running.",
-               NOT_STOPPED_STATUSES, fixed.size());
+      LOG.info("Correct {} run records with status in {} that have no actual running program. " +


Oops this needs to be "Corrected"

anew · 2017-09-21T18:17:34Z

cdap-app-fabric/src/main/java/co/cask/cdap/internal/app/services/RunRecordCorrectorService.java

-              programRunId,
-              new Throwable("Marking run record for " + programRunId + " as failed since no running program found."));
+            programStateWriter.error(programRunId,
+                                     new ProgramRunAbortedException("Marking run record for "


should this use the same message as the warning?

anew · 2017-09-21T18:18:08Z

Only two more comments about messages that slipped my radar the first time.

chtyim · 2017-09-21T22:07:24Z

Build passed https://builds.cask.co/browse/CDAP-RUT1410

anew

LGTM

chtyim · 2017-09-21T22:45:03Z

Squashed and merging.

chtyim requested a review from ecfm September 21, 2017 10:10

chtyim added the 4.3 label Sep 21, 2017

chtyim force-pushed the feature_release/CDAP-12646-fix-run-record-fixer branch from 90c5398 to af2f8f4 Compare September 21, 2017 10:19

anew reviewed Sep 21, 2017

View reviewed changes

chtyim force-pushed the feature_release/CDAP-12646-fix-run-record-fixer branch from 15f15d0 to 6aa87f0 Compare September 21, 2017 20:24

anew approved these changes Sep 21, 2017

View reviewed changes

chtyim added 2 commits September 21, 2017 15:44

(CDAP-12646) Fix the run record fixer logic to be more efficient.

416dc94

Update doc MD5.

4f1e80e

chtyim force-pushed the feature_release/CDAP-12646-fix-run-record-fixer branch from 6aa87f0 to 4f1e80e Compare September 21, 2017 22:44

chtyim merged commit 0308c0d into release/4.3 Sep 21, 2017

chtyim deleted the feature_release/CDAP-12646-fix-run-record-fixer branch September 21, 2017 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(CDAP-12646) fix run record fixer #9652

(CDAP-12646) fix run record fixer #9652

chtyim commented Sep 21, 2017

anew left a comment

anew Sep 21, 2017

chtyim Sep 21, 2017

anew Sep 21, 2017

chtyim Sep 21, 2017

anew Sep 21, 2017

chtyim Sep 21, 2017

anew Sep 21, 2017

chtyim Sep 21, 2017

anew Sep 21, 2017

chtyim Sep 21, 2017

anew Sep 21, 2017

anew Sep 21, 2017

chtyim Sep 21, 2017

chtyim commented Sep 21, 2017

anew Sep 21, 2017

anew Sep 21, 2017

anew commented Sep 21, 2017

chtyim commented Sep 21, 2017

anew left a comment

chtyim commented Sep 21, 2017

(CDAP-12646) fix run record fixer #9652

(CDAP-12646) fix run record fixer #9652

Conversation

chtyim commented Sep 21, 2017

anew 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

Choose a reason for hiding this comment

chtyim commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anew commented Sep 21, 2017

chtyim commented Sep 21, 2017

anew left a comment

Choose a reason for hiding this comment

chtyim commented Sep 21, 2017