asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "abdullah alamoudi (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 05:15:49 GMT
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 <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