Mailing-List: contact notifications-help@asterixdb.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@asterixdb.apache.org
Date: Mon, 5 Jun 2017 02:50:14 -0700
From: "abdullah alamoudi (Code Review)" <do-not-reply@asterixdb.incubator.apache.org>
CC: Jenkins <jenkins@fulliautomatix.ics.uci.edu>,
	Murtadha Hubail <hubailmor@gmail.com>,
	Ian Maxon <imaxon@apache.org>, Till Westmann <tillw@apache.org>,
	Yingyi Bu <buyingyi@gmail.com>, Luo Chen <cluo8@uci.edu>
Reply-To: bamousaa@gmail.com
Subject: Change in asterixdb[master]: Enable Component rollback in LSM Indexes
In-Reply-To: <gerrit.1496046661153.I5fe10965734b60776100580c9568a0f954d6cb58@asterix-gerrit.ics.uci.edu>
References: <gerrit.1496046661153.I5fe10965734b60776100580c9568a0f954d6cb58@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.12.7
Message-Id: <20170605095014.DEC3D2412CB@unhygienix.ics.uci.edu>
archived-at: Mon, 05 Jun 2017 09:50:19 -0000

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