asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "abdullah alamoudi (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Enable Component rollback in LSM Indexes
Date Mon, 05 Jun 2017 09:50:14 GMT
abdullah alamoudi has posted comments on this change.

Change subject: Enable Component rollback in LSM Indexes
......................................................................


Patch Set 4:

(13 comments)

https://asterix-gerrit.ics.uci.edu/#/c/1788/4/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/io/IIOManager.java
File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/io/IIOManager.java:

Line 82:      * 
> Fix WS.
Done


Line 94:      * 
> Fix WS.
Done


https://asterix-gerrit.ics.uci.edu/#/c/1788/4/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/io/IOManager.java
File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/io/IOManager.java:

PS4, Line 350: delete
> Move public static methods to a XxxUtil class?
Done


https://asterix-gerrit.ics.uci.edu/#/c/1788/4/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/IPageManager.java
File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/IPageManager.java:

PS4, Line 171: clear the page manager (cancel flush or merge)
> I couldn't understand how the implementation matches the description?  (It'
The implementation you posted from the linked metadata page manager resets the page.

the other implementation is in the append only page manager:

       if (confiscatedPage != null) {
            bufferCache.returnPage(confiscatedPage, false);
        }
        ready = false;

if we don't do this, then when we try to destroy, we first close(). if we didn't clear, then
the close will write the metadata page making the disk component a valid disk component.

if a crash happens at this point, and before the destroy removes the file, then we have a
disk component that we should not.


https://asterix-gerrit.ics.uci.edu/#/c/1788/4/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMComponent.java
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMComponent.java:

PS4, Line 74: ROLLING_BACK
> Is adding a new state into the state machine mandatory?
I thought about this when I was writing the implementation. I remember there was a case where
one would want to distinguish between the two but looking at it now, I think they can be merged!!!!
not sure though. Done


https://asterix-gerrit.ics.uci.edu/#/c/1788/4/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMDiskComponent.java
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMDiskComponent.java:

PS4, Line 69: boolean exists();
> It looks nobody calls that method, thus we probably shouldn't expose that.
Done


https://asterix-gerrit.ics.uci.edu/#/c/1788/4/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractLSMMemoryComponent.java
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractLSMMemoryComponent.java:

PS4, Line 115: IllegalStateException
> error code
illegal state!!


PS4, Line 119: IllegalStateException
> error code
illegal state!!


https://asterix-gerrit.ics.uci.edu/#/c/1788/4/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/LSMHarness.java
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/LSMHarness.java:

PS4, Line 210: {
> I wonder whether there is a better approach?
Done


Line 216:                                 failedOperation = true;
> fix this
Done


PS4, Line 218: AbstractLSMDiskComponent
> cast is not needed.
Done


PS4, Line 223: markAsValid
> markAsValid will result in a disk force, i.e., an I/O operation.  It doesn'
Done


https://asterix-gerrit.ics.uci.edu/#/c/1788/4/hyracks-fullstack/hyracks/hyracks-util/src/main/java/org/apache/hyracks/util/JSONUtil.java
File hyracks-fullstack/hyracks/hyracks-util/src/main/java/org/apache/hyracks/util/JSONUtil.java:

PS4, Line 185:  
> complete the java doc.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5fe10965734b60776100580c9568a0f954d6cb58
Gerrit-PatchSet: 4
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-Reviewer: Ian Maxon <imaxon@apache.org>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Luo Chen <cluo8@uci.edu>
Gerrit-Reviewer: Murtadha Hubail <hubailmor@gmail.com>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message