Murtadha Hubail has posted comments on this change. Change subject: ASTERIXDB-969: Redesigned recovery analysis phase to spill to disk ......................................................................
Patch Set 1: (10 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. Done Line 217: jobId2WinnerEntitiesMap.remove(logRecord.getJobId()); > let's be consistent: either use jobid or logRecord.getJobId(). Done Line 225: if (needToFreeMemroy()) { > Typo: needToFreeMemory Done Line 574: if (!jobRecoveryFile.exists()) { > How can the file exist? Should this be an exception? I changed it to be an exception if it already exists. Line 834: if (preparedForSearch) { > This method is called only once per winnerEntitySet. So, this check seems u This is just a protection in case someone calls it twice by mistake. I removed it anyway. Line 909: if (needToFreeMemroy()) { > Are these check and free operation necessary? It seems that the same operat The next step after this line allocates a buffer that can hold this in memory partition. If there isn't enough memory, it will throw an error. That's why this check is needed. I can eliminate it by writing the in-memory cached entities to the file channel one by one, but I don't think it is worth it. Line 921: File partitionFile = createJobRecoveryFile(jobId, getPartitonName(partitionMaxLSN)); > typo: getPartitionName Done Line 923: try (FileOutputStream fileOutputstream = new FileOutputStream(partitionFile, false); > what is this try for? This is try with resources that automatically closes the file channel at the end of the try block Line 926: fileChannel.write(buffer); > busy waiting and write everything Done Line 927: fileChannel.force(true); > force seems not necessary. If there is a crash, anyway everything will be d Removed. -- 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: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Young-Seok Kim <[email protected]> Gerrit-HasComments: Yes
