Yingyi Bu has posted comments on this change. Change subject: Improve Reading from and Writing to Metadata Pages ......................................................................
Patch Set 4: (46 comments) I prefer to still keep the LSM prefix a while. That at least make the code base consistent. Otherwise, half class/interfaces do not have LSM, while the other half have. Once we remove all in-place stuff, we can write a script to automatically remove LSMs in class names. https://asterix-gerrit.ics.uci.edu/#/c/1476/2//COMMIT_MSG Commit Message: PS2, Line 7: Pages Pages -> pages PS2, Line 7: Reading Reading -> reading Writing -> writing PS2, Line 9: IComponentMetadata IComponentMetadata. PS2, Line 12: The The --> the PS2, Line 14: flush flush --> merge? 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 PS3, Line 67: IDiskComponent c : im AbstractDiskComponent -> ILSMDiskComponent PS3, Line 104: IDiskComponent> rever AbstractDiskComponent -> ILSMDiskComponent PS3, Line 109: List<IDiskComponent> mergableComponents = new ArrayList<>(); AbstractDiskComponent -> ILSMDiskComponent 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 PS3, Line 96: ? extends ILSMComponen <? extends ILSMComponent> -> <ILSMComponents> 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 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 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 PS3, Line 48: ? extends ILSMComponent> <? extends ILSMComponent> --> ILSMComponent 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? Isn't it one call back per partition? PS3, Line 53: public why public? Move to a util class if it's needed. PS3, Line 56: 1L What does -1L mean? Use a constant with meaning name for it? PS3, Line 69: move to a util class? PS3, Line 84: private move to a util class? PS3, Line 105: synchronized why synchronized is 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? It's log flusher's job to guarantee sequential log writes. Our convention is to have one call back per partition. 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? PS3, Line 59: using ObjectUtils? 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? 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? 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/ArrayBackedValueStorage.java File hyracks-fullstack/hyracks/hyracks-data/hyracks-data-std/src/main/java/org/apache/hyracks/data/std/util/ArrayBackedValueStorage.java: PS3, Line 78: result ObjectUtils? PS3, Line 89: } ObjectUtils? 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. PS3, Line 89: false using instanceof? PS3, Line 97: using Arrays.equals 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. 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?? IDiskComponent --> ILSMDiskComponent Just to be consistent PS3, Line 673: IDiskComponent IDiskComponent -> ILSMDiskComponent 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? 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 "LSM" is used all over the places, thus we'd better be consistent. At the moment we remove all in-place stuff, we can remove the "LSM" prefix but it's OK to keep it. 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. PS2, Line 35: IMO, the interface factory should return an interface object. It looks weird to return an abstract class. 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? 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? 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. What does it mean? 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? 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? PS3, Line 75: getRefCount getRefCount --> getFileRefCount() The original name was clearer to me. Otherwise, I would think it is the ref count for the disk component. 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? 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? -- 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
