Yingyi Bu has posted comments on this change. Change subject: Fix upsert deadlock and upsert with filtered primary only bug ......................................................................
Patch Set 3: (7 comments) https://asterix-gerrit.ics.uci.edu/#/c/1759/3//COMMIT_MSG Commit Message: PS3, Line 7: deadlock Add a detailed description to explain what's the deadlock and what the change fixes? If it's too verbose, maybe create an issue? https://asterix-gerrit.ics.uci.edu/#/c/1759/3/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: PS3, Line 218: enter As we discussed before, enter/exit are internal details and probably shouldn't be exposed. Instead, you can expose per-frame based "logical" operations in ILSMHarness to the caller. In this way, caller won't mess up with enter/exit. https://asterix-gerrit.ics.uci.edu/#/c/1759/3/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMComponentFilterFactory.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMComponentFilterFactory.java: PS3, Line 32: getFilterCmpFactories By the class name ILSMComponentFilterFactory, it is supposed to only create an ILSMComponentFilter. This doesn't look to be the right place for getFilterCmpFactories(), move getFilterComparator() to ILSMComponentFilter since it's a property of the filter? If ILSMComponentFilter is one per partition, maybe add a method called getFilterComparator() into it since it doesn't need to create a factory? https://asterix-gerrit.ics.uci.edu/#/c/1759/3/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: PS3, Line 205: ( I remember that we discussed before that enter/exit are details that each operation should take care of and they shouldn't be exposed to the caller. https://asterix-gerrit.ics.uci.edu/#/c/1759/3/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMIndexAccessor.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMIndexAccessor.java: PS3, Line 39: */ enter/exist are internal to a LSM operation and hence shouldn't be exposed to the caller. https://asterix-gerrit.ics.uci.edu/#/c/1759/3/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: PS3, Line 54: getIndexTuple Add javadocs for each interface method PS3, Line 58: getFilterCmp getFilterCmp - > getFilterComparator -- To view, visit https://asterix-gerrit.ics.uci.edu/1759 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic702d6e1984ea33408c360b6f522f493848cbd87 Gerrit-PatchSet: 3 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Jianfeng Jia <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
