>From Murtadha Hubail <[email protected]>: Attention is currently required from: Peeyush Gupta, Ali Alsuliman. Murtadha Hubail has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559 )
Change subject: [NO ISSUE][TX] Partition level atomicity implementation ...................................................................... Patch Set 9: (31 comments) File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/PrimaryIndexOperationTracker.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/b95a6e6e_718521f1 PS9, Line 171: LOGGER.debug how about for now we do this with atomic indexes but keep the illegal state exception for non-atomic? this way we will make sure we didn't regress anything related to non-atomic indexes. We can remove the illegal state completely at a later point. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/ad3b1aa5_bc14a09f PS9, Line 260: triggerCommit commit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/f8bd490d_d685c3dd PS9, Line 262: remove this line https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/72af16f8_5c5cb5c7 PS9, Line 267: FlushOperation flush : lastFlushOperation.values( Since lastFlushOperation is a map now, its iterator won't have a stable order and therefore the checkpoints won't be called in the right order. I don't think you are using the map key, so changing it to a list might do the trick. Also, you might want to call it uncommittedFlushes https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/8d81f65b_15fbf61e PS9, Line 282: triggerAbort abort File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/LSMInsertDeleteOperatorNodePushable.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/f22c2ce6_1466e7d6 PS9, Line 222: commitAtomicInsertDelete Might be worth it to think to think about what it means if this call throws an exception. It might be fine and the global atomic txn later will make sure we are in a stable state, but just wanted to highlight. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/a7a92fd2_138997e2 PS9, Line 247: PrimaryIndexOperationTracker opTracker = : ((PrimaryIndexOperationTracker) ((ILSMIndex) index).getOperationTracker()); : LogRecord logRecord = new LogRecord(); : opTracker.triggerScheduleFlush(logRecord); : List<FlushOperation> flushes = new ArrayList<>(opTracker.getScheduledFlushes()); : LSMIndexUtil.waitFor(flushes); : INcApplicationContext appCtx = : (INcApplicationContext) ctx.getJobletContext().getServiceContext().getApplicationContext(); : opTracker.triggerCommit(appCtx.getIndexCheckpointManagerProvider()); I think it is better to have this logic inside opTracker.commit similar to opTracker.abort. After that, all 3 operators that have this duplicate code will just call opTracker.commit. Not sure if we need to change the synchronization a bit to make it work. If it is the case, let's defer this to another change. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/c5412d0d_e23860a0 PS9, Line 255: appCtx.getIndexCheckpointManagerProvider() we can do this in another change, but let's just pass the IndexCheckpointManagerProvider to the PrimaryIndexOperationTracker File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/ioopcallbacks/AtomicLSMIndexIOOperationCallbackFactory.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/9e2aab3b_a9279eee PS9, Line 35: add serialVersionUID. According to our test, it should be: private static final long serialVersionUID = -2617830712731546932L; File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Dataset.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/f92fe07b_07ace6f2 PS9, Line 509: isAtomicStatementsEnabled isAtomic File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/LSMPrimaryInsertOperatorNodePushable.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/c1674995_fe03747d PS9, Line 168: searchCallback instanceof LockThenSearchOperationCallback Not for this patch, but we probably should make this cleaner and not based on instanceof. We can include the release() in an interface and have our no-op do nothing. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/2dfe8a60_1bfffea3 PS9, Line 256: if (((ILSMIndex) indexForUniquessCheck).isAtomicStatementsEnabled()) { : Map<String, Object> indexAccessorOpContextParameters = new HashMap<>(); : indexAccessorOpContextParameters.put(HyracksConstants.ATOMIC_OP_CONTEXT, true); : lsmAccessorForUniqunessChecks[i].getOpContext().setParameters(indexAccessorOpContextParameters); : } extract this as its own method and if possible refactor it with the similar code in the other operators. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/edaea503_8275d0fa PS9, Line 346: this.failed = true; failed = true; https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/aca21788_8a75508f PS9, Line 358: PrimaryIndexOperationTracker opTracker = : ((PrimaryIndexOperationTracker) ((ILSMIndex) index).getOperationTracker()); : LogRecord logRecord = new LogRecord(); : opTracker.triggerScheduleFlush(logRecord); : List<FlushOperation> flushes = new ArrayList<>(opTracker.getScheduledFlushes()); : LSMIndexUtil.waitFor(flushes); : INcApplicationContext appCtx = : (INcApplicationContext) ctx.getJobletContext().getServiceContext().getApplicationContext(); : opTracker.triggerCommit(appCtx.getIndexCheckpointManagerProvider()); same comments as before File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/LSMPrimaryUpsertOperatorNodePushable.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/ce9bc40c_df609f5b PS9, Line 234: abstractModCallback instanceof AbstractIndexModificationOperationCallback not for this patch, but let's try to avoid instanceof here too File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/IndexInsertUpdateDeleteOperatorNodePushable.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/0bfd3db4_48a0f407 PS9, Line 208: this.failed = true; failed = true; File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree-column/src/main/java/org/apache/hyracks/storage/am/lsm/btree/column/dataflow/LSMColumnBTreeLocalResource.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/576293fa_14cdcaf0 PS9, Line 69: false why false? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/f3390eb7_28d4ae5b PS9, Line 79: false why false? File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree-column/src/main/java/org/apache/hyracks/storage/am/lsm/btree/column/dataflow/LSMColumnBTreeLocalResourceFactory.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/b57152b2_811c1772 PS9, Line 59: false why false? File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/dataflow/LSMBTreeLocalResource.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/85288b9e_4809ae21 PS9, Line 63: isAtomicStatementsEnabled atomic https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/39e03531_32b721bb PS9, Line 149: isAtomicStatementsEnabled let's just call it atomic https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/4d00d429_a8dc45ce PS9, Line 156: appendToJson shouldn't atomic be added here? File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/dataflow/LSMBTreeLocalResourceFactory.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/45f0d041_12d09a80 PS9, Line 52: isAtomicStatementsEnabled atomic File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTree.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/cb7c0358_12b989a1 PS9, Line 108: false why false? File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMIndex.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/2f083f15_dd42bba1 PS9, Line 46: isAtomicStatementsEnabled isAtomic https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/b81b1f77_769b2d66 PS9, Line 48: cleanupForAbortedStatement rename to "abort" + add java docs to what it means https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/81c65b4c_0bbaaaf6 PS9, Line 117: validateTemporaryDiskComponents rename to "commit" + add java docs to what it means File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractLSMIndex.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/f3a02f00_6296cd6a PS9, Line 116: isAtomicStatementsEnabled atomic https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/92e9ca58_1ebe6f42 PS9, Line 178: isAtomicStatementsEnabled isAtomic https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/199705fa_8c9b5257 PS9, Line 347: (!isAtomicStatementsEnabled || (ctxParameters != null : && (boolean) ctxParameters.getOrDefault(HyracksConstants.ATOMIC_OP_CONTEXT, false)) extractMethod and use it below too https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/fe82e53a_a3aca3b7 PS9, Line 609: LOGGER.log(Level.INFO, "DiskB ", temporaryDiskComponents.size(), c.toString()); if you'd like to keep this log for now, change it to something like this: LOGGER.debug("adding new temporary disk component to index {}; current count: {}", c, temporaryDiskComponents.size()); -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559 To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: I8ed0fac37e026c909c986e6d844c3fae3833911e Gerrit-Change-Number: 17559 Gerrit-PatchSet: 9 Gerrit-Owner: Peeyush Gupta <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Peeyush Gupta <[email protected]> Gerrit-Attention: Peeyush Gupta <[email protected]> Gerrit-Attention: Ali Alsuliman <[email protected]> Gerrit-Comment-Date: Sat, 03 Jun 2023 23:35:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
