>From Peeyush Gupta <[email protected]>: Attention is currently required from: Murtadha Hubail, Ali Alsuliman. Peeyush Gupta 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 10: Code-Review+1 (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/c39e0ac2_5f2842c9 PS9, Line 171: LOGGER.debug > how about for now we do this with atomic indexes but keep the illegal state > exception for non-atomic […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/eb1116e3_124e7ad5 PS9, Line 260: triggerCommit > commit Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/49b9a306_0ec74056 PS9, Line 262: > remove this line Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/e62d4744_d213a735 PS9, Line 267: FlushOperation flush : lastFlushOperation.values( > Since lastFlushOperation is a map now, its iterator won't have a stable order > and therefore the chec […] lastFlushOperation map is to store just the last flush done on an index. Checkpoint is created only for the last component flushed. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/33aad85f_17fc3999 PS9, Line 282: triggerAbort > abort Done File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/LSMInsertDeleteOperatorNodePushable.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/e1fe78c5_661c3164 PS9, Line 222: commitAtomicInsertDelete > Might be worth it to think to think about what it means if this call throws > an exception. […] Ack https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/cba968b7_2108ef05 PS9, Line 255: appCtx.getIndexCheckpointManagerProvider() > we can do this in another change, but let's just pass the > IndexCheckpointManagerProvider to the Prim […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/07c13df5_418785c5 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. […] Done File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/ioopcallbacks/AtomicLSMIndexIOOperationCallbackFactory.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/89a50b3a_ab1d2105 PS9, Line 35: > add serialVersionUID. According to our test, it should be: […] Done File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Dataset.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/70b3fa5d_55ebb097 PS9, Line 509: isAtomicStatementsEnabled > isAtomic Done File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/LSMPrimaryInsertOperatorNodePushable.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/8d346247_2ee3074b PS9, Line 168: searchCallback instanceof LockThenSearchOperationCallback > Not for this patch, but we probably should make this cleaner and not based on > instanceof. […] Ack https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/80105ed4_7d4202d9 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 operat […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/d61cc338_2e3eef5f PS9, Line 346: this.failed = true; > failed = true; Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/aa38b5d2_0fc7c140 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 Done File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/LSMPrimaryUpsertOperatorNodePushable.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/0f154e49_db05c4cd PS9, Line 234: abstractModCallback instanceof AbstractIndexModificationOperationCallback > not for this patch, but let's try to avoid instanceof here too Ack 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/36c63ffe_08d2266e PS9, Line 208: this.failed = true; > failed = true; Done 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/5b055ef1_cb840f2a PS9, Line 69: false > why false? I was not sure to enable atomic statements on columnar dataset. I have updated the code now. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/6eb8d551_215f1c5d PS9, Line 79: false > why false? Done 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/384eec4e_e6fedd88 PS9, Line 59: false > why false? Done 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/b96bbe19_9df5dca3 PS9, Line 63: isAtomicStatementsEnabled > atomic Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/45395fe4_5f964018 PS9, Line 149: isAtomicStatementsEnabled > let's just call it atomic Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/c922ee06_51a79c15 PS9, Line 156: appendToJson > shouldn't atomic be added here? Done 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/ebc88838_25a113d2 PS9, Line 52: isAtomicStatementsEnabled > atomic Done 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/36bdede0_5286a1cf PS9, Line 108: false > why false? Done 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/489de6c8_c91750ef PS9, Line 46: isAtomicStatementsEnabled > isAtomic Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/94f64dfe_653950b0 PS9, Line 48: cleanupForAbortedStatement > rename to "abort" + add java docs to what it means Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/1007d9b5_5c82b3a2 PS9, Line 117: validateTemporaryDiskComponents > rename to "commit" + add java docs to what it means Done 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/f3060ebd_8316a36d PS9, Line 116: isAtomicStatementsEnabled > atomic Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/36faadcd_e1e8c769 PS9, Line 178: isAtomicStatementsEnabled > isAtomic Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/8aa0d60a_517a7158 PS9, Line 347: (!isAtomicStatementsEnabled || (ctxParameters != null : && (boolean) ctxParameters.getOrDefault(HyracksConstants.ATOMIC_OP_CONTEXT, false)) > extractMethod and use it below too Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17559/comment/07275840_301dc14e 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: […] Done -- 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: 10 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: Murtadha Hubail <[email protected]> Gerrit-Attention: Ali Alsuliman <[email protected]> Gerrit-Comment-Date: Mon, 05 Jun 2023 20:30:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Murtadha Hubail <[email protected]> Gerrit-MessageType: comment
