Yingyi Bu has posted comments on this change. Change subject: Deadlock-free locking protocol is enabled Change-Id: Ie58ae2f519baa53599e99b51bd61ea5f8366dafd ......................................................................
Patch Set 1: (35 comments) The commit message needs to follow the format style: https://cwiki.apache.org/confluence/display/ASTERIXDB/Formatting "Leave a blank line after the subject line." Detailed comments are inlined. https://asterix-gerrit.ics.uci.edu/#/c/825/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/base/RuleCollections.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/base/RuleCollections.java: Line 294: //physicalRewritesAllLevels.add(new IntroduceInstantLockSearchCallbackRule()); Remove this line instead of commenting out. Also, remove the class IntroduceInstantLockSearchCallbackRule as well. https://asterix-gerrit.ics.uci.edu/#/c/825/1/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/AsterixLSMInsertDeleteOperatorNodePushable.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/AsterixLSMInsertDeleteOperatorNodePushable.java: Line 48: private int lastFlushedTupleIdx; annotate the meaning of each: private int currentTupleIdx; private int lastFlushedTupleIdx; private boolean flushedPartialTuples; Line 93: flushedPartialTuples = false; annotate why they need to be reset to 0/false for each frame. https://asterix-gerrit.ics.uci.edu/#/c/825/1/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/AsterixLSMPrimaryUpsertOperatorNodePushable.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/AsterixLSMPrimaryUpsertOperatorNodePushable.java: Line 256: * flushes tuples in a frame from lastFlushedTupleIdx(inclusive) to currentTupleIdx(exclusive) is it possible to factor-out/unify the code of flushPartialFrame for both insert-delete and upsert? e.g., using a separate class/field-member to manage that? Line 260: FrameUtils.appendToWriter(writer, appender, accessor, i); This method of this class is never called. Because in PrimaryIndexModificationOperationCallback, you cast operatorNodePushable to AsterixLSMInsertDeleteOperatorNodePushable. I'm not sure why there is no type-casting error shown up. Do we have any upsert tests? https://asterix-gerrit.ics.uci.edu/#/c/825/1/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/locking/ConcurrentLockManager.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/locking/ConcurrentLockManager.java: Line 51: public static final int NIL = -1; Can you let the two variable names have more difference? It's very easy to get to use NILL for NIL or vice versa. Line 360: if (reqSlot == NILL) { NIL? Line 373: if (reqSlot != NILL) { NIL? Line 542: while (holder != NILL) { NIL? Line 561: jobSlot = new Long(jobArenaMgr.allocate()); Will this lead to many object creations back-and-forth? Because releaseLocks() remove to entry for jobThreadId. Line 583: if (resSlot == NILL) { NIL? Line 644: while (holder != NILL) { NIL? Line 664: while (resSlot != NILL) { NIL? Line 673: return NILL; NIL? Line 690: while (holder != NILL) { NIL? Line 729: if (nextForJob != NILL) { NIL? Line 732: if (prevForJob == NILL) { NIL? Line 750: reqArenaMgr.setNextRequest(request, NILL); NIL? Line 751: if (waiter == NILL) { NIL? Line 816: reqArenaMgr.setPrevJobRequest(newRequest, NILL); NIL? Line 823: long next = reqArenaMgr.getNextRequest(head); NIL? Line 833: long prev = cur; NIL? Line 836: if (cur == NILL) { NIL? Line 865: while (prev != NILL) { NIL? Line 867: if (holder == NILL) { NIL? Line 883: while (holder != NILL) { NIL? Line 952: try { per-tuple-based objection creation? Because this method is called in lock/unlock. Line 961: while (resSlot != NILL) { NIL? Line 965: while (reqSlot != NILL) { NIL? Line 988: if (findLockInJobQueue(dsId, entityHashValue, jobThreadId, lockMode) == NILL) { NIL? Line 1014: return NILL; NIL? Line 1021: while (holder != NILL) { NIL? Line 1032: return NILL; NIL? https://asterix-gerrit.ics.uci.edu/#/c/825/1/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/locking/DummyLockManager.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/locking/DummyLockManager.java: Line 37: * @author tillw why do we still need this class? It looks never used anywhere. https://asterix-gerrit.ics.uci.edu/#/c/825/1/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/locking/LockManager.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/locking/LockManager.java: Line 54: Why should we still keep/maintain this class? This lock manager is no longer used anywhere. -- To view, visit https://asterix-gerrit.ics.uci.edu/825 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie58ae2f519baa53599e99b51bd61ea5f8366dafd Gerrit-PatchSet: 1 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Young-Seok Kim <kiss...@gmail.com> Gerrit-Reviewer: Ian Maxon <ima...@apache.org> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Murtadha Hubail <hubail...@gmail.com> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: Yingyi Bu <buyin...@gmail.com> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
