Wail Alkowaileet has posted comments on this change. Change subject: [ASTERIXDB-2422][STO] Introduce compressed storage ......................................................................
Patch Set 3: (43 comments) https://asterix-gerrit.ics.uci.edu/#/c/2857/2/asterixdb/asterix-app/src/test/resources/runtimets/compressionSqlpp.xml File asterixdb/asterix-app/src/test/resources/runtimets/compressionSqlpp.xml: PS2, Line 54: > remove Done https://asterix-gerrit.ics.uci.edu/#/c/2857/2/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/config/StorageProperties.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/config/StorageProperties.java: PS2, Line 51: STORAGE_COMPRESSION_SCHEME(STRING, " > remove Done PS2, Line 52: > This value should be referenced from somewhere else (e.g. CompressionManage CompressionManager is not visible here. It seems that we put string constants (e.g replication strategy in ReplicationProperties)? PS2, Line 94: > The default compression scheme for storage Done https://asterix-gerrit.ics.uci.edu/#/c/2857/2/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/exceptions/ErrorCode.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/exceptions/ErrorCode.java: PS2, Line 183: public static final int UNKNOWN_COMPRESSION_SCHEME = 1095 > This shouldn't be needed. Replace it by illegal state exception Done 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: C > no such parameter Done PS2, Line 42: > This parameter doesn't seem right. Why would the compression manager know a Done https://asterix-gerrit.ics.uci.edu/#/c/2857/2/asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties File asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties: PS2, Line 170: 1095 = Unknown compression scheme %1$s. Supported schemes are %2$s > shouldn't be needed Done 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: CompressionSchemePara > This seems to be a parameter and not a hint. Done https://asterix-gerrit.ics.uci.edu/#/c/2857/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/compression/CompressionManager.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/compression/CompressionManager.java: PS2, Line 38: private static final > remove Done PS2, Line 42: * New compression schem > remove Done PS2, Line 45: ible with the previ > shouldn't be needed Done PS2, Line 46: > Call this from a static block. Done PS2, Line 58: : @Override : publi > Remove this Done PS2, Line 66: } catch (IllegalAccessException | InstantiationException e) { : throw new IllegalStateException("Failed to instantiate compressor/decompressor: " + scheme, e); : } > Let's also add Java's "deflate" compressor, so users can enable compression >From the API, Deflater is not thread-safe. Example: byte[] output = new byte[100]; Deflater compresser = new Deflater(); compresser.setInput(input); compresser.finish(); int compressedDataLength = compresser.deflate(output); compresser.end(); Unless we're willing to create an object (Deflater, Inflater) for each compress/uncompress call? or synchronize it? PS2, Line 66: } catch (IllegalAccessException | InstantiationException e) { : throw new IllegalStateException("Failed to instantiate compressor/decompressor: " + scheme, e); : } > From the experiments results, it appears that snappy always outperformed th Done PS2, Line 89: true if it > you shouldn't need this Done PS2, Line 116: me != null ? ddlSche > This should be illegal state exception Done PS2, Line 131: > The CC/NC will fail to start up and the only way to figure out what's happe Done 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: The required buffer size > s/computeCompressBufferSize/computeCompressedBufferSize/ Done PS2, Line 58: > s/uncompressedBuffer/uBuffer/ Done PS2, Line 58: > s/compressedBuffer/cBuffer/ Done PS2, Line 80: > s/uncompressedBuffer/uBuffer/ Done PS2, Line 80: > s/compressedBuffer/cBuffer/ Done PS2, Line 81: > s/length/cLength/ Done https://asterix-gerrit.ics.uci.edu/#/c/2857/2/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/compression/ICompressorDecompressorFactory.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/compression/ICompressorDecompressorFactory.java: PS2, Line 40: > remove Done https://asterix-gerrit.ics.uci.edu/#/c/2857/2/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/exceptions/ErrorCode.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/exceptions/ErrorCode.java: PS2, Line 152: public static final int ONE_TUPLE_RANGEMAP_EXPECTED = 116; : public static final int NO_RANGEMAP_PRODUCED = 117; : public static final int RANGEMAP_NOT_FOUND = 118; > None of these should be user visible errors. Let's remove them and replace Done https://asterix-gerrit.ics.uci.edu/#/c/2857/2/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/io/FileReference.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/io/FileReference.java: PS2, Line 33: ODeviceHandle > With the addition of the lafFileRef, this isn't a single FileReference anym Done PS2, Line 113: istrationTime == 0) { > I know this is called only once, but having the implementation call createI Done PS2, Line 140: > If this can be accessed by multiple threads, then this isn't thread safe. removed https://asterix-gerrit.ics.uci.edu/#/c/2857/2/hyracks-fullstack/hyracks/hyracks-api/src/main/resources/errormsg/en.properties File hyracks-fullstack/hyracks/hyracks-api/src/main/resources/errormsg/en.properties: PS2, Line 135: 116 = One tuple rangemap is expected : 117 = No range map produced for parallel sort : 118 = Range map was not found for parallel sort > Shouldn't be needed Done 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: yFie > Use a NOOP ICompressorDecompressorFactory instead of null? Done 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 227: ARNING: flushing the metadata page sho > remove Done PS2, Line 235: erCache.returnPage(confiscatedPage, false); > remove Done PS2, Line 262: > Could you expand the comment with more details? It's a mistake. It's 2 (root and metadata pages) and other 2 pages for bloom filter not for the index. 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 245: //Use putInQueue instead > remove Done PS2, Line 289: ressedPageWriter.abo > Could we have a NOOP compressedPageWriter instead? Or just a PageWriter? Done 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: > Wondering if we could have something less physical here. But I'm not sure a I'm not sure that I get it. But the function toJSON will only serialize a JSON with three fields (class#getSimpleName(), serialVersionUID, class#getName()) 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: ameFilter) > This parameter seems unfortunate. Why does the getter of the file reference Done PS2, Line 382: essorDecompres > s/isCompressable/isCompressible/ ? Done https://asterix-gerrit.ics.uci.edu/#/c/2857/2/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/BufferCache.java File hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/BufferCache.java: PS2, Line 60: > I won't review this class now as addressing the comments might make more ch I abstracted all file operations from the BufferCache. BufferedFileHandle and CompressedFileHandle have different implementations for each file operation. 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 IPageWriteFailureCallback failureCallback; : > move this above the DEBUG members? Done https://asterix-gerrit.ics.uci.edu/#/c/2857/2/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/file/compress/CompressedFileManager.java File hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/file/compress/CompressedFileManager.java: PS2, Line 254: > contain Done -- 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: 3 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
