Mailing-List: contact notifications-help@asterixdb.incubator.apache.org;
 run by ezmlm
Precedence: bulk
Reply-To: dev@asterixdb.incubator.apache.org
Date: Wed, 28 Oct 2015 19:02:13 -0700
From: "Murtadha Hubail (Code Review)"
 <do-not-reply@asterixdb.incubator.apache.org>
CC: Jenkins <jenkins@fulliautomatix.ics.uci.edu>,
	Young-Seok Kim <kisskys@gmail.com>
Reply-To: hubailmor@gmail.com
Subject: Change in asterixdb[master]: ASTERIXDB-969: Redesigned recovery
 analysis phase to spill t...
In-Reply-To: 
 <gerrit.1444981101092.Ide2b346c2ad498d7595e71bae890362c2143d301@asterix-gerrit.ics.uci.edu>
References: 
 <gerrit.1444981101092.Ide2b346c2ad498d7595e71bae890362c2143d301@asterix-gerrit.ics.uci.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.8.4
Message-Id: <20151029020213.33645240C3B@unhygienix.ics.uci.edu>

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