Wail Alkowaileet has posted comments on this change.

Change subject: [ASTERIXDB-2422][STO] Introduce compressed storage
......................................................................


Patch Set 18:

(4 comments)

https://asterix-gerrit.ics.uci.edu/#/c/2857/17/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:

PS17, Line 256:  consider the index is not empty
              :             return IBufferCache.INVALID_PAGEID;
              :         }
> we don't allow a tree to be <2 pages anymore?
We need at least 2 pages to consider the tree as not empty (metadata page and 
root). Therefore, <2 means it's invalid.


https://asterix-gerrit.ics.uci.edu/#/c/2857/17/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/impls/AbstractTreeIndex.java
File 
hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/impls/AbstractTreeIndex.java:

PS17, Line 303:  if (hasFailed()) {
              :                 throw HyracksDataException.create(getFailure());
              :             }
              :             bufferCache.finishQueue();
> why move this? is it the case that finishQueue doesn't have the metadata pa
The buffer cache is guarded against writing a deleted file (it is no-op). So 
there no reason for waiting for the queue. finishQueue for the metadata page is 
called at AppendOnlyLinkedMetadataPageManager#close() not in in the 
bulk-loader. I'm not sure if that was the case previously.


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

PS17, Line 392: 
              :     private boolean isCompressible(String fileName) {
              :         return !fileName.endsWith(BLOOM_FILTER_SUFFIX) && 
!fileName.endsWith(DELETE_TREE_SUFFIX);
              :     }
> should this also apply to rtrees ?
Yes. I Missed this one.
I was supporting the compression for RTrees but after running few experiments, 
it was not beneficial (doubles are mostly not compressible).

I can change that in a separate patch as the current RTree will have NoOp 
compressor.


https://asterix-gerrit.ics.uci.edu/#/c/2857/17/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/compression/file/LAFWriter.java
File 
hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/compression/file/LAFWriter.java:

PS17, Line 111: );
> how many pages can you have confiscated at once for the LAF file?
It depends on how many extra pages and the buffer cache page size. If there are 
100 extra data pages and each page can hold 10 LAF entries, then we need 10 
pages for the LAF file.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idde6f37c810c30c7f1a5ee8bcbc1e3e5f4410031
Gerrit-PatchSet: 18
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Wail Alkowaileet <[email protected]>
Gerrit-Reviewer: Anon. E. Moose #1000171
Gerrit-Reviewer: Dmitry Lychagin <[email protected]>
Gerrit-Reviewer: Ian Maxon <[email protected]>
Gerrit-Reviewer: Jenkins <[email protected]>
Gerrit-Reviewer: Michael Blow <[email protected]>
Gerrit-Reviewer: Murtadha Hubail <[email protected]>
Gerrit-Reviewer: Till Westmann <[email protected]>
Gerrit-Reviewer: Wail Alkowaileet <[email protected]>
Gerrit-HasComments: Yes

Reply via email to