Murtadha Hubail has posted comments on this change. Change subject: [ASTERIXDB-2422][STO] Introduce compressed storage ......................................................................
Patch Set 2: (26 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 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: //Default compression scheme is none remove PS2, Line 52: none This value should be referenced from somewhere else (e.g. CompressionManager#NONE) PS2, Line 94: The compression scheme for the storage The default compression scheme for storage 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 INVALID_COMPRESSION_CONFIG = 1095 This shouldn't be needed. Replace it by illegal state exception 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 = Invalid compression configuration (%1$s = %2$s). Valid values are: %3$s shouldn't be needed 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: //Compression Schemes remove PS2, Line 42: //Default configurations remove PS2, Line 45: AlgebricksException shouldn't be needed PS2, Line 46: registerSchemes(); Call this from a static block. PS2, Line 58: if (REGISTERED_SCHEMES.size() > 0) { : return; : } Remove this PS2, Line 66: REGISTERED_SCHEMES.put("lz4", LZ4CompressorDecompressorFactory.class); : //Version 1.4.1 : REGISTERED_SCHEMES.put("lz4hc", LZ4HCCompressorDecompressorFactory.class); >From the experiments results, it appears that snappy always outperformed these >two. I propose that we remove them. However, we still need to maintain the >nice extensibility here, and I believe addressing Till's comment of adding a >NOOP compression will help in maintaining it. PS2, Line 89: registerSchemes(); you shouldn't need this PS2, Line 116: CompilationException This should be illegal state exception PS2, Line 131: CompilationException The CC/NC will fail to start up and the only way to figure out what's happening is to look thru the logs. So, just use an illegal state exception 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: public remove 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 WRONG_SIZE_AFTER_UNCOMPRESS = 116; : public static final int FAILED_READ_COMPLETE_PAGE = 117; : public static final int UNPREPARED_COMPRESSED_WRITE = 118; None of these should be user visible errors. Let's remove them and replace their usage by illegal state exceptions. 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: FileReference With the addition of the lafFileRef, this isn't a single FileReference anymore. Think about introducing a wrapper for compressed files that will actually have two file references if possible. If that works, you wont be needing most of the checks in isCompressed here. PS2, Line 113: compressorDecompressorFactory.createInstance(); I know this is called only once, but having the implementation call createInstance every time if this getter called more than once could cause trouble down the road. PS2, Line 140: if (lafFileRef == null) { If this can be accessed by multiple threads, then this isn't thread safe. 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 = Failed to uncompress {file: %1$s, pageId: %2$s at offset: %3$s with size: %4$s bytes} : 117 = Failed to read complete page: %1$s of %2$s bytes : 118 = Unprepared compressed-write for page ID = %1$s Shouldn't be needed 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: //Prepare to write the compressed page remove PS2, Line 235: //Finalize the writing of the compressed pages remove 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 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: BufferCache I won't review this class now as addressing the comments might make more changes here. However, this class is already unmaintainable as is. Adding all of this code to it will make it just worse. I know none of this is your fault but if there is an opportunity to move the compression related code to a class which the BufferCache could simply call for compressed files, it will be better. Also, if there is any stateless code that can be extracted to say a CompressionUtil, let's do that too. 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: contains contain -- 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: Michael Blow <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
