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 <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Young-Seok Kim <[email protected]> Gerrit-HasComments: Yes
