Murtadha Hubail has posted comments on this change. Change subject: [ASTERIXDB-1952][TX][IDX] Filter logs pt.2 ......................................................................
Patch Set 26: (24 comments) Please start with my comment on IModificationOperationCallback. In addition to these comments, the new FILTER logs will have to replicated. You will need to modify LogManagerWithReplication to include them in the logs to be replicated and modify RemoteLogsProcessor to accept them. https://asterix-gerrit.ics.uci.edu/#/c/1857/26/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/RecoveryManager.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/RecoveryManager.java: PS26, Line 280: @SuppressWarnings({"squid:MethodCyclomaticComplexity","squid:S134"}) remove this. It is better that we get the warning so that we know this is a good candidate to refactor. https://asterix-gerrit.ics.uci.edu/#/c/1857/26/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/LogRecord.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/LogRecord.java: Line 127: case LogType.ENTITY_COMMIT: writeEntityResource PS26, Line 168: writeEntityInfo writeEntityValue PS26, Line 169: buffer.putInt(resourcePartition); : buffer.putInt(datasetId); remove PS26, Line 179: writeEntityInfoNoPK writeEntityResource Line 275: case LogType.ENTITY_COMMIT: readEntityResource PS26, Line 293: readEntityNoPKInfo readEntityResource PS26, Line 294: RecordReadStatus updStatus = return PS26, Line 330: readEntityInfo readEntityValue PS26, Line 335: resourcePartition = buffer.getInt(); : datasetId = buffer.getInt(); remove PS26, Line 350: readEntityNoPKInfo readEntityResource PS26, Line 351: //attempt to read in the resourcePartition, dsid, PK hash and PK length fix comment PS26, Line 352: ENTITYCOMMIT_UPDATE_HEADER_LEN this isn't correct. You need a new one for entity resource length, then adjust ENTITYCOMMIT_UPDATE_HEADER_LEN https://asterix-gerrit.ics.uci.edu/#/c/1857/26/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/LSMPrimaryUpsertOperatorNodePushable.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/LSMPrimaryUpsertOperatorNodePushable.java: PS26, Line 170: true revert https://asterix-gerrit.ics.uci.edu/#/c/1857/26/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/AbstractIndexModificationOperationCallback.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/AbstractIndexModificationOperationCallback.java: PS26, Line 127: public void after(ITupleReference newValue) have an explicit call to log the filter's new min and max PS26, Line 135: catch (ACIDException e) { : throw new HyracksDataException(e); : } ACIDException is a runtime exception now, you don't need to wrap it. https://asterix-gerrit.ics.uci.edu/#/c/1857/26/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/ophelpers/IndexOperation.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/ophelpers/IndexOperation.java: PS26, Line 28: FILTER_MOD you shouldn't need this https://asterix-gerrit.ics.uci.edu/#/c/1857/26/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMHarness.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMHarness.java: PS26, Line 229: /** : * Update the filter with the value in the passed tuple, also give the tuple to the modification callback : * : * @param ctx : * @throws HyracksDataException : */ : void updateFilter(ILSMIndexOperationContext ctx, ITupleReference tuple, boolean callback) : throws HyracksDataException; You shouldn't need this https://asterix-gerrit.ics.uci.edu/#/c/1857/26/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/LSMHarness.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/LSMHarness.java: PS26, Line 721: @Override : public void updateFilter(ILSMIndexOperationContext ctx, ITupleReference tuple, boolean callback) : throws HyracksDataException { : if (!callback) { : updateFilter(ctx, tuple); : return; : } : : if (!lsmIndex.isMemoryComponentsAllocated()) { : lsmIndex.allocateMemoryComponents(); : } : ctx.getModificationCallback().after(tuple); : lsmIndex.updateFilter(ctx, tuple); : } remove https://asterix-gerrit.ics.uci.edu/#/c/1857/26/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/LSMTreeIndexAccessor.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/LSMTreeIndexAccessor.java: PS26, Line 214: public void updateFilter(ITupleReference tuple, boolean callback) throws HyracksDataException { : ctx.setOperation(IndexOperation.UPSERT); : lsmHarness.updateFilter(ctx, tuple, callback); : } remove https://asterix-gerrit.ics.uci.edu/#/c/1857/26/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/IModificationOperationCallback.java File hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/IModificationOperationCallback.java: PS26, Line 62: void after(ITupleReference after) throws HyracksDataException; If you do this here, then you have to stick to the contract of this interface and make this call in non-LSM indexes after modification (exactly like the #before call). You need to introduce ILSMModificationOperationCallback interface that extends this interface and is LSM filter aware and add an explicit API to notify filter value changing (new min tuple, new max tuple) then implement the logging in Asterix. You will need to modify AbstractLSMIndex#updateFilter and call the new API to notify filter changing but only when the filter is really changing (i.e. don't generate a log if the new tuple doesn't really change the filter min/max. https://asterix-gerrit.ics.uci.edu/#/c/1857/26/hyracks-fullstack/hyracks/hyracks-test-support/src/main/java/org/apache/hyracks/storage/am/btree/AbstractModificationOperationCallbackTest.java File hyracks-fullstack/hyracks/hyracks-test-support/src/main/java/org/apache/hyracks/storage/am/btree/AbstractModificationOperationCallbackTest.java: PS26, Line 104: @Override : public void after(ITupleReference tuple) throws HyracksDataException { : Assert.assertEquals(0, cmp.compare(AbstractModificationOperationCallbackTest.this.tuple, tuple)); : } you shouldn't need this https://asterix-gerrit.ics.uci.edu/#/c/1857/26/hyracks-fullstack/hyracks/hyracks-test-support/src/main/java/org/apache/hyracks/storage/am/common/TestOperationCallback.java File hyracks-fullstack/hyracks/hyracks-test-support/src/main/java/org/apache/hyracks/storage/am/common/TestOperationCallback.java: PS26, Line 69: @Override : public void after(ITupleReference tuple) throws HyracksDataException { : // Do nothing. : } you shouldn't need this https://asterix-gerrit.ics.uci.edu/#/c/1857/26/hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-am-lsm-btree-test/src/test/java/org/apache/hyracks/storage/am/lsm/btree/LSMBTreeUpdateInPlaceTest.java File hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-am-lsm-btree-test/src/test/java/org/apache/hyracks/storage/am/lsm/btree/LSMBTreeUpdateInPlaceTest.java: PS26, Line 180: @Override : public void after(ITupleReference tuple) { : : } you shouldn't need this -- To view, visit https://asterix-gerrit.ics.uci.edu/1857 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie9e7795d9c8c212e8610dcb9bb5d26ec9fbbee8a Gerrit-PatchSet: 26 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Ian Maxon <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Luo Chen <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
