Murtadha Hubail has posted comments on this change. Change subject: [ASTERIXDB-2422][STO] Introduce compressed storage ......................................................................
Patch Set 9: (26 comments) https://asterix-gerrit.ics.uci.edu/#/c/2857/9/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/io/PersistedResourceRegistry.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/io/PersistedResourceRegistry.java: PS9, Line 253: Block remove block Line 274: final String resourceId; do this at the beginning: if(json != null) { return deserialize(json); } PS9, Line 279: resourceId = getResourceId(defaultClass); : } : try { : Class<? extends IJsonSerializable> clazz = getResourceClass(resourceId); : //Using static method (fromJson) : Method method = clazz.getMethod(DESERIALIZATION_METHOD, IPersistedResourceRegistry.class, JsonNode.class); : return (IJsonSerializable) method.invoke(null, this, json); : } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException e) { : throw HyracksDataException.create(e); : } extract method e.g. deserializeDefault https://asterix-gerrit.ics.uci.edu/#/c/2857/9/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/SqlppExecutionWithCompresisionTest.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/SqlppExecutionWithCompresisionTest.java: PS9, Line 51: compression_sqlpp.xm If a new test case that might cause problems to compression is added to the full suite and not added here, then it won't be detected. I suggest that we run the full suite and for the new test cases, you can explicitly specify the compression scheme in the DDLs. Also, you might get rid of the new test files that were added just to add the compression scheme in the DDLs. If you have some tests that don't make sense to add to the original suite then move that small subset to its own test class. https://asterix-gerrit.ics.uci.edu/#/c/2857/9/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/compression/scheme-none/scheme-none.1.ddl.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/compression/scheme-none/scheme-none.1.ddl.sqlpp: PS9, Line 40: WS https://asterix-gerrit.ics.uci.edu/#/c/2857/9/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/compression/scheme-snappy/scheme-snappy.1.ddl.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/compression/scheme-snappy/scheme-snappy.1.ddl.sqlpp: PS9, Line 40: WS https://asterix-gerrit.ics.uci.edu/#/c/2857/9/asterixdb/asterix-app/src/test/resources/runtimets/rebalance.xml File asterixdb/asterix-app/src/test/resources/runtimets/rebalance.xml: PS9, Line 80: Replication Rebalance https://asterix-gerrit.ics.uci.edu/#/c/2857/9/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: PS9, Line 26: validate validates https://asterix-gerrit.ics.uci.edu/#/c/2857/9/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/util/ConfigurationUtil.java File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/util/ConfigurationUtil.java: Line 34: remove empty line https://asterix-gerrit.ics.uci.edu/#/c/2857/9/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: PS9, Line 63: hints.add(new CompressionSchemeParam()); We need to get rid of this completely. PS9, Line 133: CompressionSchemeParam This shouldn't be treated as a hint. https://asterix-gerrit.ics.uci.edu/#/c/2857/9/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: PS9, Line 50: //Version 1.7.1.1 remove https://asterix-gerrit.ics.uci.edu/#/c/2857/9/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: PS9, Line 245: //Use putInQueue instead remove https://asterix-gerrit.ics.uci.edu/#/c/2857/9/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/AbstractBufferedFileIOManager.java File hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/AbstractBufferedFileIOManager.java: PS9, Line 38: unexepected unexpected PS9, Line 43: protected private PS9, Line 47: volatile this doesn't need to be volatile anymore Line 48: private boolean hasOpen; volatile PS9, Line 168: open opened PS9, Line 173: hasBeenOpen hasBeenOpened PS9, Line 283: throwErrorException Errors in java are scary. Rename to something that doesn't have the word "error" (e.g. throwException) https://asterix-gerrit.ics.uci.edu/#/c/2857/9/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: PS9, Line 782: hasBeenOpen you have to make this flag volatile for this work. https://asterix-gerrit.ics.uci.edu/#/c/2857/9/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: Line 242: remove https://asterix-gerrit.ics.uci.edu/#/c/2857/9/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/compression/file/CompressedFileManager.java File hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/compression/file/CompressedFileManager.java: PS9, Line 210: private void ensureState(State expectedState) { : if (state != expectedState) { : throw new IllegalStateException( : "Expecting the state to be " + expectedState + ". Currently it is " + state); : } : } : : private void ensureState(State expectedState1, State expectedState2) { : if (state != expectedState1 && state != expectedState2) { : throw new IllegalStateException("Expecting the state to be " + expectedState1 + " or " + expectedState2 : + ". Currently it is " + state); : } : } You could replace those by a single method ensureState(EnumSet<State> expectedStates) https://asterix-gerrit.ics.uci.edu/#/c/2857/9/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/compression/file/CompressedFileReference.java File hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/compression/file/CompressedFileReference.java: PS9, Line 42: //Debug constructor : public CompressedFileReference(FileReference fileRef, ICompressorDecompressor compressorDecompressor) { : this(fileRef.getDeviceHandle(), compressorDecompressor, fileRef.getRelativePath(), : fileRef.getRelativePath() + ".dic"); : } Please remove this. You could use the main constructor on the test or extend this class on the test and add the new constructor that. https://asterix-gerrit.ics.uci.edu/#/c/2857/9/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: PS9, Line 79: synchronized (cachedFrames) { Please review the synchronization on this class. Synchronizing on ConcurrentHashMap usually defeats its purpose. Also, make sure that recycledFrames is accessed in thread-safe manner since ArrayDeque isn't thread-safe. I couldn't completely follow the reasoning behind synchronizing on some instances and not others. Also, if this class isn't meant to be accessed by multiple threads, you may remove the synchronization and just add @NotThreadSafe. PS9, Line 155: confsicatePage confiscatePage -- 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: 9 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
