Young-Seok Kim has posted comments on this change. Change subject: ASTERIXDB-969: Redesigned recovery analysis phase to spill to disk ......................................................................
Patch Set 1: (10 comments) Please address comments. https://asterix-gerrit.ics.uci.edu/#/c/458/1/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/recovery/RecoveryManager.java File asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/recovery/RecoveryManager.java: Line 213: if (jobId2WinnerEntitiesMap.containsKey(jobId)) { Here, jobId is not updated. jobId should be set before it's used. Line 217: jobId2WinnerEntitiesMap.remove(logRecord.getJobId()); let's be consistent: either use jobid or logRecord.getJobId(). Seems jobId better than calling logRecord.getJobId() repeatedly Line 225: if (needToFreeMemroy()) { Typo: needToFreeMemory Line 574: if (!jobRecoveryFile.exists()) { How can the file exist? Should this be an exception? Line 834: if (preparedForSearch) { This method is called only once per winnerEntitySet. So, this check seems useless.? Line 909: if (needToFreeMemroy()) { Are these check and free operation necessary? It seems that the same operations in analysis phase are sufficient? Line 921: File partitionFile = createJobRecoveryFile(jobId, getPartitonName(partitionMaxLSN)); typo: getPartitionName Line 923: try (FileOutputStream fileOutputstream = new FileOutputStream(partitionFile, false); what is this try for? Line 926: fileChannel.write(buffer); busy waiting and write everything Line 927: fileChannel.force(true); force seems not necessary. If there is a crash, anyway everything will be deleted and start over, right? -- To view, visit https://asterix-gerrit.ics.uci.edu/458 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ide2b346c2ad498d7595e71bae890362c2143d301 Gerrit-PatchSet: 1 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Young-Seok Kim <[email protected]> Gerrit-HasComments: Yes
