asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Murtadha Hubail (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: ASTERIXDB-969: Redesigned recovery analysis phase to spill t...
Date Thu, 29 Oct 2015 02:02:13 GMT
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 <hubailmor@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Murtadha Hubail <hubailmor@gmail.com>
Gerrit-Reviewer: Young-Seok Kim <kisskys@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message