asterixdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Young-Seok Kim (Code Review)" <do-not-re...@asterix-gerrit.ics.uci.edu>
Subject Change in hyracks[master]: Make LSM bulkload append-only and write-once.
Date Thu, 13 Aug 2015 04:53:46 GMT
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

Mime
View raw message