Change in asterixdb[master]: [ASTERIXDB-2204][STO] Fix implementations and usages of IInd...

["abdullah alamoudi (Code Review)" <do...@asterixdb.incubator.apache.org>]

abdullah alamoudi has posted comments on this change.

Change subject: [ASTERIXDB-2204][STO] Fix implementations and usages of IIndexCursor
......................................................................


Patch Set 6:

(8 comments)

https://asterix-gerrit.ics.uci.edu/#/c/2324/6/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/IIndexOperationContext.java
File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/IIndexOperationContext.java:

PS6, Line 38:  /**
            :      * Release resources associated with the context and prepare it for the next use
            :      */
            :     void reset();
> Do we need to change the name "reset" to align with "close" in cursors?
I think that we should look at this interface in a subsequent change... There are a few things here. The context doesn't have an open call and so, I am not sure if this might be confusing.


https://asterix-gerrit.ics.uci.edu/#/c/2324/6/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTree.java
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTree.java:

PS6, Line 197:  if (found) {
             :                 ctx.getCurrentMutableBTreeAccessor().upsertIfConditionElseInsert(tuple,
             :                         AntimatterAwareTupleAcceptor.INSTANCE);
             :                 return true;
             :             }
> Why move these statements here?
It is basically the same thing except that instead of having memCursor.close() be in two places, it is in a single place in the finally clause.


https://asterix-gerrit.ics.uci.edu/#/c/2324/6/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTreeCursorInitialState.java
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTreeCursorInitialState.java:

PS6, Line 42:     private boolean isScan;
> What's the usage of this "isScan"? I remembered previous there is a similar
Done


https://asterix-gerrit.ics.uci.edu/#/c/2324/6/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTreePointSearchCursor.java
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTreePointSearchCursor.java:

PS6, Line 205: closeCursors
> why call closeCursors instead of destroyCursors here?
Done


https://asterix-gerrit.ics.uci.edu/#/c/2324/6/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTreeSearchCursor.java
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTreeSearchCursor.java:

PS6, Line 53: isScan
> I see the usage here...But I suggest we should change the name of isScan to
Done


https://asterix-gerrit.ics.uci.edu/#/c/2324/6/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTreeWithBuddySearchCursor.java
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTreeWithBuddySearchCursor.java:

PS6, Line 41:   super.doDestroy();
            :         currentCursor = 0;
> Do we need to destroy cursors here?
super.doDestroy() takes care of it.


https://asterix-gerrit.ics.uci.edu/#/c/2324/6/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/ComponentReplacementContext.java
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/ComponentReplacementContext.java:

PS6, Line 216: 
             :     @Override
             :     public void destroy() throws HyracksDataException {
             :         throw new UnsupportedOperationException();
             :     }
             : }
> why through exception here instead of no op?
Done


https://asterix-gerrit.ics.uci.edu/#/c/2324/6/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-rtree/src/main/java/org/apache/hyracks/storage/am/lsm/rtree/impls/LSMRTree.java
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-rtree/src/main/java/org/apache/hyracks/storage/am/lsm/rtree/impls/LSMRTree.java:

PS6, Line 111:     public ILSMDiskComponent doFlush(ILSMIOOperation operation) throws HyracksDataException {
> This method seems to be too complicated after the change...I think we shoul
100% Agree
This is a tough task and for that reason, I postponed... But will do it...


-- 
To view, visit https://asterix-gerrit.ics.uci.edu/2324
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I98a7a8b931eb24dbe11bf2bdc61b754ca28ebdf9
Gerrit-PatchSet: 6
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: abdullah alamoudi <ba...@gmail.com>
Gerrit-Reviewer: Anon. E. Moose #1000171
Gerrit-Reviewer: Jenkins <je...@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Luo Chen <cl...@uci.edu>
Gerrit-Reviewer: Michael Blow <mb...@apache.org>
Gerrit-Reviewer: Murtadha Hubail <mh...@apache.org>
Gerrit-Reviewer: Taewoo Kim <ta...@uci.edu>
Gerrit-Reviewer: Taewoo Kim <wa...@gmail.com>
Gerrit-Reviewer: Till Westmann <ti...@apache.org>
Gerrit-Reviewer: abdullah alamoudi <ba...@gmail.com>
Gerrit-HasComments: Yes