Mailing-List: contact notifications-help@asterixdb.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@asterixdb.apache.org
Date: Wed, 7 Jun 2017 08:29:44 -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: <20170607152944.9A28E240C57@unhygienix.ics.uci.edu>
archived-at: Wed, 07 Jun 2017 15:29:49 -0000

abdullah alamoudi has posted comments on this change.

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


Patch Set 9:

(12 comments)

1. we already use the code in exitComponents to get rid of the files.
2. the clear is needed in case of rollback while new component is being created through flush or merge.

It is needed because the rollback doesn't see the new component being created. if we don't clear, then we will write a valid new component to disk instead of writing an invalid component in case of failure.

https://asterix-gerrit.ics.uci.edu/#/c/1788/9/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/dataflow/ComponentRollbackTest.java
File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/dataflow/ComponentRollbackTest.java:

PS9, Line 83: ComponentRollbackTest
> Fix all Thread.sleep(...) according to the sonar cube suggestion?
Done


https://asterix-gerrit.ics.uci.edu/#/c/1788/9/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/CorrelatedPrefixMergePolicy.java
File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/CorrelatedPrefixMergePolicy.java:

PS9, Line 280: || immutableComponents.get(i).getState() == ComponentState.DECOMMISSIONING
> why decommissioning implies isMergeOngoing==true?
yes. decommissioning on memory component means flush is finishing but still ongoing. decommissioning on disk component means merge is finishing but still ongoing


https://asterix-gerrit.ics.uci.edu/#/c/1788/9/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/utils/IoUtil.java
File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/utils/IoUtil.java:

PS9, Line 19: control.nc
> move this to hyracks-util?
Done. can maximumly put it in hyracks-api. since it needs to see the ErrorCode.


https://asterix-gerrit.ics.uci.edu/#/c/1788/9/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:

PS9, Line 121: IllegalStateException
> Error code?
This is illegal state which should never happen according to the code. you can see that the one in the middle where writerCount > 0 can happen and so I created an error code for it. but for those, I think illegalstate is the right type of exception.


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

PS9, Line 138: immutableComponents.get(i).getState() == ComponentState.DECOMMISSIONING
> The code reads weird:
Correct


https://asterix-gerrit.ics.uci.edu/#/c/1788/9/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:

PS9, Line 217: failedOperation
> pull if(failedOperation) out to reduce nested levels.
Done


PS9, Line 261:   if (opType == LSMOperationType.ROLLBACK) {
> Move the if branch to be CASE in side the next switch(opType)?
Done


PS9, Line 262: lsmIndex.getDiskComponents().remove(c);
> Do that inside lsmIndex so that readers can easily track which component(s)
Done


PS9, Line 496: if (newComponent != null)
> reverse the if condition to reduce nesting levels.
Done


PS9, Line 690: rollback
> change the method name to logRollback()?
Done


PS9, Line 686: try {
             :                 for (ILSMComponent c : ctx.getComponentHolder()) {
             :                     if (c.getType() == LSMComponentType.DISK) {
             :                         // persist rollback in case of crash
             :                         ((ILSMDiskComponent) c).rollback();
             :                     }
             :                 }
             :             }
> Creating a shadow file does not need to be within synchronized(opTracker).
all the rollback should be within the synchronized block since the enterComponent doesn't do anything but increment the number or readers.

we can't allow a flush/merge/modify/search to enter while we're rolling back.

All the actual work (beside logging the operation) happens in exitComponent()

we just can't start doing anything in enter component since we don't know if we will be able to enter all components.


PS9, Line 693: finally {
             :                 exitComponents(ctx, LSMOperationType.ROLLBACK, null, false);
             :             }
> exitComponents(...) shouldn't under the umbrella of synchronized(opTracker)
See above comment!


-- 
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: 9
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