abdullah alamoudi has posted comments on this change. Change subject: Improve Reading from and Writing to Metadata Pages ......................................................................
Patch Set 4: (39 comments) https://asterix-gerrit.ics.uci.edu/#/c/1476/3/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/CorrelatedPrefixMergePolicy.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/CorrelatedPrefixMergePolicy.java: PS3, Line 63: IDiskComponent> immut > AbstractDiskComponent -> ILSMDiskComponent Done PS3, Line 67: IDiskComponent c : im > AbstractDiskComponent -> ILSMDiskComponent Done PS3, Line 104: IDiskComponent> rever > AbstractDiskComponent -> ILSMDiskComponent Done PS3, Line 109: List<IDiskComponent> mergableComponents = new ArrayList<>(); > AbstractDiskComponent -> ILSMDiskComponent Done https://asterix-gerrit.ics.uci.edu/#/c/1476/3/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/ioopcallbacks/AbstractLSMIOOperationCallback.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/ioopcallbacks/AbstractLSMIOOperationCallback.java: PS3, Line 81: IDiskComponent newCom > AbstractDiskComponent -> ILSMDiskComponent Done PS3, Line 96: ? extends ILSMComponen > <? extends ILSMComponent> -> <ILSMComponents> I would leave this as it is since it allows both: 1. List<ILSMComponent> 2.List<ILSMDiskComponent> We need to use it for both cases https://asterix-gerrit.ics.uci.edu/#/c/1476/3/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/ioopcallbacks/LSMBTreeWithBuddyIOOperationCallback.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/ioopcallbacks/LSMBTreeWithBuddyIOOperationCallback.java: PS3, Line 35: throws HyracksDataException { > AbstractDiskComponent -> ILSMDiskComponent Done https://asterix-gerrit.ics.uci.edu/#/c/1476/3/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/ioopcallbacks/LSMInvertedIndexIOOperationCallback.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/ioopcallbacks/LSMInvertedIndexIOOperationCallback.java: PS3, Line 40: throws HyracksDataExc > AbstractDiskComponent -> ILSMDiskComponent Done https://asterix-gerrit.ics.uci.edu/#/c/1476/3/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/ioopcallbacks/LSMRTreeIOOperationCallback.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/ioopcallbacks/LSMRTreeIOOperationCallback.java: PS3, Line 40: throws HyracksDataExc > AbstractDiskComponent -> ILSMDiskComponent Done PS3, Line 48: ? extends ILSMComponent> > <? extends ILSMComponent> --> ILSMComponent I will leave this as it is since it allows for the list to be: 1. List<ILSMComponent> 2. List<anything that implements ILSMComponent> We need to use the two cases https://asterix-gerrit.ics.uci.edu/#/c/1476/3/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/PrimaryIndexLogMarkerCallback.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/PrimaryIndexLogMarkerCallback.java: PS3, Line 49: synchronized > why synchronized is needed? Done. PS3, Line 53: public > why public? it is only needed for a test case PS3, Line 56: 1L > What does -1L mean? Done. PS3, Line 69: > move to a util class? Private and only needed made static for a test case PS3, Line 84: private > move to a util class? private and only made static for a use case. it is not used outside this class PS3, Line 105: synchronized > why synchronized is needed? Done. It is not needed https://asterix-gerrit.ics.uci.edu/#/c/1476/4/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/PrimaryIndexLogMarkerCallback.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/PrimaryIndexLogMarkerCallback.java: PS4, Line 49: synchronized > Why synchronized is need? Done https://asterix-gerrit.ics.uci.edu/#/c/1476/3/hyracks-fullstack/algebricks/algebricks-common/src/main/java/org/apache/hyracks/algebricks/common/utils/Pair.java File hyracks-fullstack/algebricks/algebricks-common/src/main/java/org/apache/hyracks/algebricks/common/utils/Pair.java: PS3, Line 48: } > Using ObjectUtils? was generated using the IDE. the one before was broken. we will get rid of this class anyway so let's leave it for now :) Plus ObjectUtils has deprecated their equals and hashCode PS3, Line 59: > using ObjectUtils? This was generated by the IDE/ we will get rid of this class anyway so let's leave it. Plus ObjectUtils has deprecated their equals and hashCode https://asterix-gerrit.ics.uci.edu/#/c/1476/3/hyracks-fullstack/algebricks/algebricks-common/src/main/java/org/apache/hyracks/algebricks/common/utils/Triple.java File hyracks-fullstack/algebricks/algebricks-common/src/main/java/org/apache/hyracks/algebricks/common/utils/Triple.java: PS3, Line 45: hash += second.hashCode() * 131; > using ObjectUtils? auto generated. let's keep it since we're removing the class soon anyway? Plus ObjectUtils has deprecated their equals and hashCode PS3, Line 55: instanceof Triple<?, ?, ?>)) { : return false; : } : Triple<?, ?, ?> triple = (Triple<?, ?, ?>) o; : if (this.first == null && triple.first != null) { : return false; : } : if (this.second == null && triple.second != null) { : return false; : } : if (t > Using ObjectUtils? autogenerated. let's leave it since we're getting rid of the class anyway. Plus ObjectUtils has deprecated their equals and hashCode https://asterix-gerrit.ics.uci.edu/#/c/1476/3/hyracks-fullstack/hyracks/hyracks-data/hyracks-data-std/src/main/java/org/apache/hyracks/data/std/util/GrowableArray.java File hyracks-fullstack/hyracks/hyracks-data/hyracks-data-std/src/main/java/org/apache/hyracks/data/std/util/GrowableArray.java: PS3, Line 86: > line 85 to line 87 will never be executed. Done PS3, Line 89: false > using instanceof? Done PS3, Line 97: > using Arrays.equals doesn't have rangeEqual https://asterix-gerrit.ics.uci.edu/#/c/1476/3/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/freepage/AppendOnlyLinkedMetadataPageManager.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/freepage/AppendOnlyLinkedMetadataPageManager.java: PS3, Line 302: throw new HyracksDataException("The index is append only and has already been flushed to disk"); > Use error code. Done https://asterix-gerrit.ics.uci.edu/#/c/1476/3/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/ExternalBTree.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/ExternalBTree.java: PS3, Line 84: be > No LSM?? Done PS3, Line 673: IDiskComponent > IDiskComponent -> ILSMDiskComponent Done https://asterix-gerrit.ics.uci.edu/#/c/1476/3/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTreeDiskComponent.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTreeDiskComponent.java: PS3, Line 33: super((IMetadataPageManager) btree.getPageManager(), filter); > Why cast is needed? because it returns a free page manager and not a metadata page manager. since a btree can be an in place tree. https://asterix-gerrit.ics.uci.edu/#/c/1476/3/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTreeDiskComponentFactory.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTreeDiskComponentFactory.java: PS3, Line 26: IDiskComponentFactory > IDiskComponentFactory -> ILSMDiskComponentFactory Done https://asterix-gerrit.ics.uci.edu/#/c/1476/2/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMDiskComponentFactory.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMDiskComponentFactory.java: PS2, Line 32: > complete the java doc. Done PS2, Line 35: > IMO, the interface factory should return an interface object. Done https://asterix-gerrit.ics.uci.edu/#/c/1476/3/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/IMemoryComponent.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/IMemoryComponent.java: PS3, Line 23: > IMemoryComponent -> ILSMMemoryComponent? Done https://asterix-gerrit.ics.uci.edu/#/c/1476/3/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractLSMIndex.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractLSMIndex.java: PS3, Line 216: AbstractMemoryComponent > AbstractMemoryComponent -> ILSMMemoryComponent? Done https://asterix-gerrit.ics.uci.edu/#/c/1476/3/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/api/IInvertedIndex.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/api/IInvertedIndex.java: PS3, Line 45: ITreeIndex getTreeIndex(); > hard to understand it from an interface perspective. Done https://asterix-gerrit.ics.uci.edu/#/c/1476/3/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/impls/LSMInvertedIndex.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/impls/LSMInvertedIndex.java: PS3, Line 1010: > why do you need this method? This is needed to get the metadata page manager of an index component. Not applicable for the lsm inverted index https://asterix-gerrit.ics.uci.edu/#/c/1476/3/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/impls/LSMInvertedIndexDiskComponent.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/impls/LSMInvertedIndexDiskComponent.java: PS3, Line 38: IMetadataPageManager > Why cast is needed? because the page manager could be for a non lsm and in that case, it will be a simple free page manager. in the case of LSM, we get the metadata page manager from the main component of the index PS3, Line 75: getRefCount > getRefCount --> getFileRefCount() I think in this context, they are both the same thing. will revert this (bummer) https://asterix-gerrit.ics.uci.edu/#/c/1476/4/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/ondisk/OnDiskInvertedIndex.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/ondisk/OnDiskInvertedIndex.java: PS4, Line 713: btree > Why should this be exposed to the outside? to get the metadata manager. https://asterix-gerrit.ics.uci.edu/#/c/1476/4/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-rtree/src/main/java/org/apache/hyracks/storage/am/lsm/rtree/impls/LSMRTreeDiskComponent.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-rtree/src/main/java/org/apache/hyracks/storage/am/lsm/rtree/impls/LSMRTreeDiskComponent.java: PS4, Line 35: IMetadataPageManager > why cast is needed? Each index has a free page manager. in the case of an LSM index, the main component's free page manager is an IMetadataPageManager. In non lsm indexes, they don't have metadata. -- To view, visit https://asterix-gerrit.ics.uci.edu/1476 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id95ef33c0a0bc1abb3fc3ecdea5611ee4acd6dfa Gerrit-PatchSet: 4 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: Yingyi Bu <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
