Murtadha Hubail has posted comments on this change. Change subject: [NO ISSUE][STO] Add consistency to flush lifecycle ......................................................................
Patch Set 16: (29 comments) https://asterix-gerrit.ics.uci.edu/#/c/2584/16/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/NCAppRuntimeContext.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/NCAppRuntimeContext.java: PS16, Line 185: AsynchronousScheduler define as inner static class PS16, Line 188: warn fatal or error PS16, Line 194: warn fatal or error https://asterix-gerrit.ics.uci.edu/#/c/2584/16/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/metadata/MetadataTxnTest.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/metadata/MetadataTxnTest.java: PS16, Line 284: final MetadataTransactionContext committedMdTxn = MetadataManager.INSTANCE.beginTransaction(); : MetadataManager.INSTANCE.addNodegroup(committedMdTxn, new NodeGroup(committedNodeGroup, ngNodes)); : MetadataManager.INSTANCE.commitTransaction(committedMdTxn); why is this needed? https://asterix-gerrit.ics.uci.edu/#/c/2584/16/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/ioopcallbacks/LSMIOOperationCallback.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/ioopcallbacks/LSMIOOperationCallback.java: PS16, Line 54: public private PS16, Line 55: public private PS16, Line 106: // check status of the flush Remove PS16, Line 181: ILSMComponent ILSMDiskComponent PS16, Line 182: ILSMDiskComponent Remove PS16, Line 215: // check status before updating remove https://asterix-gerrit.ics.uci.edu/#/c/2584/16/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/ioopcallbacks/LSMIndexIOOperationCallbackFactory.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/ioopcallbacks/LSMIndexIOOperationCallbackFactory.java: PS16, Line 38: protected private https://asterix-gerrit.ics.uci.edu/#/c/2584/16/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMIOOperation.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMIOOperation.java: PS16, Line 140: @param operation Remove https://asterix-gerrit.ics.uci.edu/#/c/2584/16/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMIOOperationCallback.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMIOOperationCallback.java: PS16, Line 87: operation java docs PS16, Line 87: getOperationGroupId is the group id always the index identifier? if so, I think we should remove the "group" from here and replace it with something that indicates the index identifier. https://asterix-gerrit.ics.uci.edu/#/c/2584/16/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMIndexOperationContext.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMIndexOperationContext.java: PS16, Line 112: void setMap(Map<String, Object> map); I think we can still do something better explicitly for flush info in Asterix implementation of this interface, but this can be done in its own change. https://asterix-gerrit.ics.uci.edu/#/c/2584/16/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractIoOperation.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractIoOperation.java: PS16, Line 87: getStatus will state be accessed from anywhere that is not synchronized? PS16, Line 124: return; why not rethrow? https://asterix-gerrit.ics.uci.edu/#/c/2584/16/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractLSMIndex.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractLSMIndex.java: PS16, Line 219: createAccessor(NoOpIndexAccessParameters.INSTANCE).scheduleFlush().sync(); what if this triggers a merge? shouldn't we wait for it as well before deactivating the index? PS16, Line 347: ScheduleFlushCalled fix PS16, Line 363: Map<String, Object> map = ctx.getMap(); : if (map != null && !map.isEmpty()) { : flushCtx.setMap(new HashMap<>(map)); : } refactor with the one below https://asterix-gerrit.ics.uci.edu/#/c/2584/16/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: PS16, Line 126: case REPLICATE: : // why would replicate ever enter a memory component? remove them PS16, Line 188: to allow for a re-attempt maybe some day :) PS16, Line 245: // a pending schedule flush replace this by when this can happen https://asterix-gerrit.ics.uci.edu/#/c/2584/16/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AsynchronousScheduler.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AsynchronousScheduler.java: PS16, Line 51: operation.getIOOpertionType() make it a switch and it would be better to extract the handling for flush https://asterix-gerrit.ics.uci.edu/#/c/2584/16/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/IoOperationExecutor.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/IoOperationExecutor.java: PS16, Line 48: Runnable remove PS16, Line 70: shutdown(); why? https://asterix-gerrit.ics.uci.edu/#/c/2584/16/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/NoOpIoOperationFailedCallback.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/NoOpIoOperationFailedCallback.java: PS16, Line 28: public private https://asterix-gerrit.ics.uci.edu/#/c/2584/16/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/BufferCache.java File hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/BufferCache.java: PS16, Line 1469: return; remove https://asterix-gerrit.ics.uci.edu/#/c/2584/16/hyracks-fullstack/hyracks/hyracks-util/src/main/java/org/apache/hyracks/util/ExitUtil.java File hyracks-fullstack/hyracks/hyracks-util/src/main/java/org/apache/hyracks/util/ExitUtil.java: PS16, Line 40: EXIT_CODE_IO_SCHEDULER_FAILED Either use EC_ or rename the rest -- To view, visit https://asterix-gerrit.ics.uci.edu/2584 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I29f7992ec6c0f71c5b63d45800b2fb590d651e4b Gerrit-PatchSet: 16 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <bamou...@gmail.com> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Luo Chen <cl...@uci.edu> Gerrit-Reviewer: Murtadha Hubail <mhub...@apache.org> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
