asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yingyi Bu (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Improve Reading from and Writing to Metadata Pages
Date Thu, 02 Feb 2017 02:40:55 GMT
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 <bamousaa@gmail.com>
Gerrit-Reviewer: Ian Maxon <imaxon@apache.org>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message