asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yingyi Bu (Code Review)" <>
Subject Change in asterixdb[master]: Enable Component rollback in LSM Indexes
Date Wed, 07 Jun 2017 00:06:27 GMT
Yingyi Bu has posted comments on this change.

Change subject: Enable Component rollback in LSM Indexes

Patch Set 9:


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()?
File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/dataflow/

PS9, Line 83: ComponentRollbackTest
Fix all Thread.sleep(...) according to the sonar cube suggestion?
File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/

PS9, Line 280: || immutableComponents.get(i).getState() == ComponentState.DECOMMISSIONING
why decommissioning implies isMergeOngoing==true?
File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/utils/

PS9, Line 19:
move this to hyracks-util?
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/

PS9, Line 121: IllegalStateException
Error code?

PS9, Line 125:  
Error code?
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/

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

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


isMergeOngoing == true
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/

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

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I5fe10965734b60776100580c9568a0f954d6cb58
Gerrit-PatchSet: 9
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