Yingyi Bu has posted comments on this change. Change subject: Separate index build from index access ......................................................................
Patch Set 4: (26 comments) https://asterix-gerrit.ics.uci.edu/#/c/1728/4/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/CorrelatedPrefixMergePolicyFactory.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/CorrelatedPrefixMergePolicyFactory.java: PS4, Line 53: setDatasetId Can we move this set call into the constructor and make datasetId a final? Eliminating setXXX as much as possible will make code easier to read. https://asterix-gerrit.ics.uci.edu/#/c/1728/4/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/DatasetLocalResource.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/DatasetLocalResource.java: PS4, Line 42: DatasetLocalResource Why IResource/DatasetLocalREsource are serializable? In principle, only the factory IResourceFactory needs to be serializable. https://asterix-gerrit.ics.uci.edu/#/c/1728/4/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/ExternalFilesIndexModificationOperatorDescriptor.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/ExternalFilesIndexModificationOperatorDescriptor.java: Line 78: switch (file.getPendingOp()) { > MAJOR SonarQube violation: add default:? https://asterix-gerrit.ics.uci.edu/#/c/1728/4/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/BTreeResourceFactoryProvider.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/BTreeResourceFactoryProvider.java: PS4, Line 112: getTypeTraits static methods should go to XxxUtil? PS4, Line 149: static methods should go to XxxUtil? PS4, Line 183: return static methods should go to XxxUtil? https://asterix-gerrit.ics.uci.edu/#/c/1728/4/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Dataset.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Dataset.java: PS4, Line 632: public Add this to IDataset? PS4, Line 639: getPrimaryTypeTraits Add this to IDataset interface? PS4, Line 668: getPrimaryComparatorFactories Add this to IDataset? PS4, Line 693: primaryBloomFilterKeyFields Add this to IDataset? https://asterix-gerrit.ics.uci.edu/#/c/1728/4/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/InvertedIndexDataflowHelperFactoryProvider.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/InvertedIndexDataflowHelperFactoryProvider.java: PS4, Line 141: IBinaryComparatorFactory static methods should go to XxxUtil? PS4, Line 154: static methods should go to XxxUtil? PS4, Line 195: } static methods should go to XxxUtil? https://asterix-gerrit.ics.uci.edu/#/c/1728/4/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/RTreeResourceFactoryProvider.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/RTreeResourceFactoryProvider.java: PS4, Line 174: static static methods should go to XxxUtil? IBinaryComparatorFactory[] doesn't seem to relate to the class name "ResourceFactory". PS4, Line 193: static methods should go to XxxUtil? PS4, Line 227: IBinaryComparatorFactoryProvider Static methods should goto XxxUtil? https://asterix-gerrit.ics.uci.edu/#/c/1728/4/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/resource/PersistentLocalResourceRepository.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/resource/PersistentLocalResourceRepository.java: PS4, Line 153: IllegalStateException Error code? https://asterix-gerrit.ics.uci.edu/#/c/1728/4/hyracks-fullstack/algebricks/algebricks-data/src/main/java/org/apache/hyracks/algebricks/data/IBinaryComparatorFactoryProvider.java File hyracks-fullstack/algebricks/algebricks-data/src/main/java/org/apache/hyracks/algebricks/data/IBinaryComparatorFactoryProvider.java: PS4, Line 51: getBinaryComparatorFactory Unify the two interface method into one? https://asterix-gerrit.ics.uci.edu/#/c/1728/4/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/ITreeIndexFrame.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/ITreeIndexFrame.java: PS4, Line 39: ; why 5->6? https://asterix-gerrit.ics.uci.edu/#/c/1728/4/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/IndexHelper.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/IndexHelper.java: PS4, Line 33: IndexHelper IndexHelper -> IndexDataflowHelper to be consistent with the interface. https://asterix-gerrit.ics.uci.edu/#/c/1728/4/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/ExternalBTreeWithBuddy.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/ExternalBTreeWithBuddy.java: PS4, Line 805: setCurrentVersion Can this setXXX be done in the constructor of the Index? In this way, we don't need to expose setXXX methods in the interface. https://asterix-gerrit.ics.uci.edu/#/c/1728/4/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ITwoPCIndex.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ITwoPCIndex.java: PS4, Line 80: setCurrentVersion Can we not expose setCurrentVersion and instead do that in the constructor of the implementation? https://asterix-gerrit.ics.uci.edu/#/c/1728/4/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/IVirtualBufferCacheProvider.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/IVirtualBufferCacheProvider.java: PS4, Line 32: List<IVirtualBufferCache> getVirtualBufferCaches(IHyracksTaskContext ctx, IFileSplitProvider fileSplitProvider) : throws HyracksDataException; : : List<IVirtualBufferCache> getVirtualBufferCaches(INCServiceContext ctx, FileSplit fileSplit) : throws HyracksDataException; I searched in the codebase, but couldn't find callers of the first two methods? https://asterix-gerrit.ics.uci.edu/#/c/1728/4/hyracks-fullstack/hyracks/hyracks-storage-common/pom.xml File hyracks-fullstack/hyracks/hyracks-storage-common/pom.xml: PS4, Line 51: dataflow Why storage should depend on dataflow? In principle, storage is library and dataflow is another peer library of hyracks. https://asterix-gerrit.ics.uci.edu/#/c/1728/4/hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-common-test/src/test/java/org/apache/hyracks/storage/common/BufferCacheTest.java File hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-common-test/src/test/java/org/apache/hyracks/storage/common/BufferCacheTest.java: PS4, Line 107: WS PS4, Line 115: WS -- To view, visit https://asterix-gerrit.ics.uci.edu/1728 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie4ea3aaa63dff8d246fa43ca7c7359729bc8cf47 Gerrit-PatchSet: 4 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-HasComments: Yes
