Murtadha Hubail has posted comments on this change. Change subject: ASTERIXDB-1011: added flow control for merge policy ......................................................................
Patch Set 3: (22 comments) Just formatting comments and suggestions :) https://asterix-gerrit.ics.uci.edu/#/c/795/3/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMMergePolicy.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMMergePolicy.java: Line 51: * WS https://asterix-gerrit.ics.uci.edu/#/c/795/3/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: Line 47: ILSMIndexAccessor accessor = (ILSMIndexAccessor) index.createAccessor(NoOpOperationCallback.INSTANCE, Remove (ILSMIndexAccessor) cast Line 51: ILSMIndexAccessor accessor = (ILSMIndexAccessor) index.createAccessor(NoOpOperationCallback.INSTANCE, Remove (ILSMIndexAccessor) cast Line 65: remove white spaces, and use multiple line comment Line 91: // [case 1] Something like this is cleaner: // [case 1] if (totalImmutableComponentCount < numComponents) { return false; } else { // [case 2 or 3] boolean isMergeOngoing = isMergeOngoing(immutableComponents); if (!isMergeOngoing) { // schedule a merge operation after making sure that all components are mergable if (!areComponentsMergable(immutableComponents)) { throw new IllegalStateException(); } ILSMIndexAccessor accessor = index.createAccessor(NoOpOperationCallback.INSTANCE, NoOpOperationCallback.INSTANCE); accessor.scheduleMerge(index.getIOOperationCallback(), immutableComponents); } return true; } Line 108: ILSMIndexAccessor accessor = (ILSMIndexAccessor) index.createAccessor(NoOpOperationCallback.INSTANCE, Remove (ILSMIndexAccessor) cast if you prefer to keep this code. Line 117: * WS Line 121: private boolean areComponentsMergable(List<ILSMComponent> immutableComponents) { You may add this as a static method in ILSMMergePolicy and use it on both polices. Line 133: * WS Line 135: * @return the index of the latest component being merged among the given list of immutable components the @return description needs to be fixed. Line 137: private boolean isMergeOngoing(List<ILSMComponent> immutableComponents) { You may add this as a static method in ILSMMergePolicy and use it on both polices. https://asterix-gerrit.ics.uci.edu/#/c/795/3/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/PrefixMergePolicy.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/PrefixMergePolicy.java: Line 52: ILSMIndexAccessor accessor = (ILSMIndexAccessor) index.createAccessor(NoOpOperationCallback.INSTANCE, Remove (ILSMIndexAccessor) cast Line 111: // case 1. remove WSs and use multiline comment Line 136: // [case 1] Something like this is cleaner: // [case 1] if (mergableImmutableComponentCount < maxToleranceComponentCount) { return false; } else { // [case 2 or 3] boolean isMergeOngoing = isMergeOngoing(immutableComponents); if (!isMergeOngoing) { // make sure that all components are of READABLE_UNWRITABLE state. if (!areComponentsReadableWritableState(immutableComponents)) { throw new IllegalStateException(); } // schedule a merge operation boolean isMergeTriggered = scheduleMerge(index); if (!isMergeTriggered) { throw new IllegalStateException(); } } return true; } Line 165: * WS Line 167: * @return the index of the latest component being merged among the given list of immutable components @return description needs to be fixed. Line 169: private boolean isMergeOngoing(List<ILSMComponent> immutableComponents) { You may add this as a static method in ILSMMergePolicy and use it on both polices. Line 183: * WS Line 202: * WS Line 206: private boolean areComponentsReadableWritableState(List<ILSMComponent> immutableComponents) { just to be consistent, rename this to areComponentsMergable. You may also add it as a static method in ILSMMergePolicy and use it on both polices. Line 217: * WS Line 252: ILSMIndexAccessor accessor = (ILSMIndexAccessor) index.createAccessor(NoOpOperationCallback.INSTANCE, Remove (ILSMIndexAccessor) cast -- To view, visit https://asterix-gerrit.ics.uci.edu/795 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ide99c022861f96cd60bc8f5795c4964ab02b3e14 Gerrit-PatchSet: 3 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Young-Seok Kim <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Young-Seok Kim <[email protected]> Gerrit-HasComments: Yes
