Mailing-List: contact notifications-help@asterixdb.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@asterixdb.apache.org
Date: Tue, 6 Jun 2017 17:06:27 -0700
From: "Yingyi Bu (Code Review)" <do-not-reply@asterixdb.incubator.apache.org>
To: abdullah alamoudi <bamousaa@gmail.com>
CC: Jenkins <jenkins@fulliautomatix.ics.uci.edu>,
	Murtadha Hubail <hubailmor@gmail.com>,
	Ian Maxon <imaxon@apache.org>, Till Westmann <tillw@apache.org>,
	Luo Chen <cluo8@uci.edu>
Reply-To: buyingyi@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: <20170607000627.334FE240C2B@unhygienix.ics.uci.edu>
archived-at: Wed, 07 Jun 2017 00:06:32 -0000

Yingyi Bu has posted comments on this change.

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


Patch Set 9:

(13 comments)

There are a couple of issues that I can think of.  It's likely that I missed sth.

1.  Can we use the similar mechanism to MERGE for deleting files/components?   E.g., ILSMIndex.subsumeComponents(...)  and inactiveDiskComponentsToBeDeleted in LSMHarness.exitComponents(...).
It feels to me that all mechanisms are there.

2.  Based on 1.,  it feels that IPageManager.clear() is not necessary.  Why MERGE doesn't need IPageManager.clear()?

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?


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?


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?


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?


PS9, Line 125:  
Error code?


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:

immutableComponents.get(i).getState() == ComponentState.DECOMMISSIONING 

implies

isMergeOngoing == true


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.


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


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

For example, add a new method called removeDiskComponent(ILSMDiskComponent c) into the ILSMIndex interface.


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


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


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).


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


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