Till Westmann has posted comments on this change. Change subject: [ASTERIXDB-2422][STO] Introduce compressed storage ......................................................................
Patch Set 2: (16 comments) first (incomplete) round of questions/comments https://asterix-gerrit.ics.uci.edu/#/c/2857/2/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/storage/ICompressionManager.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/storage/ICompressionManager.java: PS2, Line 34: hints no such parameter PS2, Line 42: isPrimary This parameter doesn't seem right. Why would the compression manager know about different index types? https://asterix-gerrit.ics.uci.edu/#/c/2857/2/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/dataset/hints/DatasetHints.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/dataset/hints/DatasetHints.java: PS2, Line 60: CompressionSchemeHint This seems to be a parameter and not a hint. https://asterix-gerrit.ics.uci.edu/#/c/2857/2/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/compression/ICompressorDecompressor.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/compression/ICompressorDecompressor.java: PS2, Line 37: computeCompressBufferSize s/computeCompressBufferSize/computeCompressedBufferSize/ PS2, Line 58: uncompressedBuffer s/uncompressedBuffer/uBuffer/ PS2, Line 58: compressedBuffer s/compressedBuffer/cBuffer/ PS2, Line 80: uncompressedBuffer s/uncompressedBuffer/uBuffer/ PS2, Line 80: compressedBuffer s/compressedBuffer/cBuffer/ PS2, Line 81: length s/length/cLength/ https://asterix-gerrit.ics.uci.edu/#/c/2857/2/hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/am/lsm/btree/LSMBTreeOperatorTestHelper.java File hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/am/lsm/btree/LSMBTreeOperatorTestHelper.java: PS2, Line 49: null Use a NOOP ICompressorDecompressorFactory instead of null? https://asterix-gerrit.ics.uci.edu/#/c/2857/2/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: PS2, Line 262: //At least there are 4 pages to consider the index is not empty Could you expand the comment with more details? https://asterix-gerrit.ics.uci.edu/#/c/2857/2/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: PS2, Line 289: compressedPageWriter Could we have a NOOP compressedPageWriter instead? Or just a PageWriter? https://asterix-gerrit.ics.uci.edu/#/c/2857/2/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/dataflow/LSMBTreeLocalResource.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/dataflow/LSMBTreeLocalResource.java: PS2, Line 134: "compressorDecompressorFactory", compressorDecompressorFactory.toJson(registry) Wondering if we could have something less physical here. But I'm not sure about the trade-offs. https://asterix-gerrit.ics.uci.edu/#/c/2857/2/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: PS2, Line 374: compressed This parameter seems unfortunate. Why does the getter of the file reference need to know if the file is compressed? PS2, Line 382: isCompressable s/isCompressable/isCompressible/ ? https://asterix-gerrit.ics.uci.edu/#/c/2857/2/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/CachedPage.java File hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/CachedPage.java: PS2, Line 53: private long compressedOffset; : private int compressedSize; move this above the DEBUG members? -- 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: 2 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Wail Alkowaileet <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
