asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "abdullah alamoudi (Code Review)" <>
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:

File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/io/

Line 82:      * 
> Fix WS.

Line 94:      * 
> Fix WS.
File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/io/

PS4, Line 350: delete
> Move public static methods to a XxxUtil class?
File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/

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.
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/

> 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
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/

PS4, Line 69: boolean exists();
> It looks nobody calls that method, thus we probably shouldn't expose that.
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/

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

PS4, Line 119: IllegalStateException
> error code
illegal state!!
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/

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

Line 216:                                 failedOperation = true;
> fix this

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

PS4, Line 223: markAsValid
> markAsValid will result in a disk force, i.e., an I/O operation.  It doesn'
File hyracks-fullstack/hyracks/hyracks-util/src/main/java/org/apache/hyracks/util/

PS4, Line 185:  
> complete the java doc.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I5fe10965734b60776100580c9568a0f954d6cb58
Gerrit-PatchSet: 4
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: abdullah alamoudi <>
Gerrit-Reviewer: Ian Maxon <>
Gerrit-Reviewer: Jenkins <>
Gerrit-Reviewer: Luo Chen <>
Gerrit-Reviewer: Murtadha Hubail <>
Gerrit-Reviewer: Till Westmann <>
Gerrit-Reviewer: Yingyi Bu <>
Gerrit-Reviewer: abdullah alamoudi <>
Gerrit-HasComments: Yes

View raw message