Mailing-List: contact dev-help@asterixdb.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@asterixdb.incubator.apache.org
Date: Wed, 12 Aug 2015 21:53:46 -0700
From: "Young-Seok Kim (Code Review)" <do-not-reply@asterix-gerrit.ics.uci.edu>
To: Ian Maxon <imaxon@apache.org>
CC: Jenkins <jenkins@fulliautomatix.ics.uci.edu>,
	Murtadha Hubail <hubailmor@gmail.com>
Reply-To: kisskys@gmail.com
Subject: Change in hyracks[master]: Make LSM bulkload append-only and
 write-once.
In-Reply-To: 
 <gerrit.1430782107564.I80fb891b5310252143854a336b591bf3f8cd4ba7@asterix-gerrit.ics.uci.edu>
References: 
 <gerrit.1430782107564.I80fb891b5310252143854a336b591bf3f8cd4ba7@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: <20150813045346.4F978240F88@unhygienix.ics.uci.edu>

Young-Seok Kim has posted comments on this change.

Change subject: Make LSM bulkload append-only and write-once.
......................................................................


Patch Set 17:

(10 comments)

I went over all files and left comments. 
Please address them.

https://asterix-gerrit.ics.uci.edu/#/c/255/17/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/edu/uci/ics/hyracks/storage/am/lsm/invertedindex/impls/LSMInvertedIndex.java
File hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/edu/uci/ics/hyracks/storage/am/lsm/invertedindex/impls/LSMInvertedIndex.java:

Line 819:             //component.getInvIndex().create();
let's remove this


Line 900:     }
This method is no-op:
So, either remove or do no-op literally.


https://asterix-gerrit.ics.uci.edu/#/c/255/17/hyracks/hyracks-storage-am-rtree/src/main/java/edu/uci/ics/hyracks/storage/am/rtree/frames/RTreeNSMInteriorFrame.java
File hyracks/hyracks-storage-am-rtree/src/main/java/edu/uci/ics/hyracks/storage/am/rtree/frames/RTreeNSMInteriorFrame.java:

Line 40:     public static final int childPtrSize = 4;
Why is this public?


Line 192:     }
This one is very similar to the existing method above and seems to be easily factored out by having a private function.


https://asterix-gerrit.ics.uci.edu/#/c/255/17/hyracks/hyracks-storage-am-rtree/src/main/java/edu/uci/ics/hyracks/storage/am/rtree/impls/RTree.java
File hyracks/hyracks-storage-am-rtree/src/main/java/edu/uci/ics/hyracks/storage/am/rtree/impls/RTree.java:

Line 1038:                 //queue.offer(frontier.page);
should go away


Line 1067:             if (toRoot && !propagated && level < nodeFrontiers.size() - 1) {
checking propagated is useless since propagated is always false if toRoot is true.


https://asterix-gerrit.ics.uci.edu/#/c/255/17/hyracks/hyracks-storage-common/src/main/java/edu/uci/ics/hyracks/storage/common/buffercache/ClockPageReplacementStrategy.java
File hyracks/hyracks-storage-common/src/main/java/edu/uci/ics/hyracks/storage/common/buffercache/ClockPageReplacementStrategy.java:

Line 41:         this.maxAllowedNumPages = new AtomicInteger(maxAllowedNumPages);
why there are two maxAllowedNumPages? Both of them don't change the value initially assigned.


Line 81:         if (maxAllowedNumPages.get() == 0)
When this value can be 0?


Line 102:             clockPtr.set(clockPtr.incrementAndGet() % (numPages.get()-1));
what is this -1 for?


Line 118:     }
When is addPage used instead of allocatePage()?
What the usage difference between the two methods?


-- 
To view, visit https://asterix-gerrit.ics.uci.edu/255
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I80fb891b5310252143854a336b591bf3f8cd4ba7
Gerrit-PatchSet: 17
Gerrit-Project: hyracks
Gerrit-Branch: master
Gerrit-Owner: Ian Maxon <imaxon@apache.org>
Gerrit-Reviewer: Ian Maxon <imaxon@apache.org>
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