Yingyi Bu has posted comments on this change. Change subject: Introduce IStorageComponentProvider ......................................................................
Patch Set 5: (59 comments) Publish my pending comments on patch set 5. Will continue to review other files in patch set 6. https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/ExternalDataLookupPOperator.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/ExternalDataLookupPOperator.java: Line 77: this.retainMissing = retainNull; retainNull -> retainMissing https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/InvertedIndexPOperator.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/InvertedIndexPOperator.java: Line 252: IIndexDataflowHelperFactory dataflowHelperFactory = dataset.indexDataflowHelperFactory(metadataProvider, indexDataflowHelperFactory -> getIndexDataflowHelperFactory Line 259: dataset.searchCallbackFactory(metadataProvider.getStorageComponentProvider(), secondaryIndex, searchCallBackFactory -> getSearchCallBackFactory https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AccessMethodJobGenParams.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AccessMethodJobGenParams.java: Line 111: new VariableReferenceExpression(keyVar)); reformat? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/common/APIFramework.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/common/APIFramework.java: Line 42: import org.apache.asterix.common.exceptions.AsterixException; unused import? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/APIServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/APIServlet.java: Line 72: IStatementExecutorFactory statementExecutorFactory, format? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/java/AsterixJavaClient.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/java/AsterixJavaClient.java: Line 76: statementExecutorFactory, format? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/external/ExternalLibraryUtils.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/external/ExternalLibraryUtils.java: Line 176: adapter.getAdapterIdentifier().getName()); format? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/NCAppRuntimeContext.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/NCAppRuntimeContext.java: Line 39: import org.apache.asterix.common.config.CompilerProperties; created ASTERIXDB-1774 for this. Line 193: txnProperties); format? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java: Line 732: MetadataTransactionContext mdTxnCtx) throws CompilationException { format? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-app/src/main/java/org/apache/asterix/drivers/AsterixWebServer.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/drivers/AsterixWebServer.java: Line 38: new StorageComponentProvider())), "/*"); format? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-app/src/main/java/org/apache/asterix/file/DataverseOperations.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/file/DataverseOperations.java: Line 31: public class DataverseOperations { add a new line? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/CCApplicationEntryPoint.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/CCApplicationEntryPoint.java: Line 269: ccExtensionManager.getStorageComponentProvider()); format? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/GlobalRecoveryManager.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/GlobalRecoveryManager.java: Line 62: remove the additional new line https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-app/src/main/java/org/apache/asterix/util/ExtensionUtils.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/util/ExtensionUtils.java: Line 37: private ExtensionUtils() { add a new line https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-app/src/test/resources/metadata/testsuite.xml File asterixdb/asterix-app/src/test/resources/metadata/testsuite.xml: Line 426: <expected-error>ASX1027</expected-error> I think our convention is to only put messages here instead of error codes. https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-app/src/test/resources/runtimets/testsuite.xml File asterixdb/asterix-app/src/test/resources/runtimets/testsuite.xml: Line 3320: <expected-error>ASX1016</expected-error> I think our convention is to only put messages here instead of error codes. https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml File asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml: Line 3122: <expected-error>ASX1016</expected-error> messages, rather than error codes. https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/IApplicationContextInfo.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/IApplicationContextInfo.java: Line 69: void setGlobalRecoveryMaanger(IGlobalRecoveryMaanger globalRecoveryMaanger); IMO, setters are evil, which prevent clean code.... Why do we need this setter here? Does the instance of GlobalRecoveryManager really need to change during the lifetime of an instance? Maanger --> Manager https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/LSMInvertedIndexInsertDeleteOperatorDescriptor.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/LSMInvertedIndexInsertDeleteOperatorDescriptor.java: Line 54: IPageManagerFactory iPageManagerFactory) { iPageManagerFactory -> pageManagerFactory I don't think we need the i-prefix for an object that implements an interface. https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/LSMTreeInsertDeleteOperatorDescriptor.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/LSMTreeInsertDeleteOperatorDescriptor.java: Line 58: ISearchOperationCallbackFactory searchOpCallbackProvider, IPageManagerFactory iPageManagerFactory) { iPageManagerFactory -> pageManagerFactory https://asterix-gerrit.ics.uci.edu/#/c/1451/5/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: Line 69: public static final int COMPILATION_NOT_IMPLEMENTED_FOR_DATASET_TYPE = 1013; Why 1013, 1014, 1017, 1018, 1019 and 1020 are never used anywhere? Line 70: public static final int COMPILATION_NOT_IMPLEMENTED_FOR_INDEX_TYPE = 1014; NOT_IMPLEMENTED needs to be specific, e.g., what is not implemented. Line 73: public static final int COMPILATION_NOT_SUPPORTED_FOR_INDEX_TYPE = 1017; NOT_SUPPORTED_FOR_INDEX_TYPE -> UNSUPPORTED_INDEX_TYPE Line 74: public static final int COMPILATION_SECONDARY_INDEX_EXPECTED = 1018; What SECONDARY_INDEX_EXPECTED mean? NO_SECONDARY_INDEX? Line 75: public static final int COMPILATION_NOT_SUPPORTED_FOR_INDEX_NAME = 1019; NOT_SUPPORTED_FOR_INDEX_NAME --> UNSUPPORTED_INDEX_NAME Line 77: public static final int COMPILATION_NOT_SUPPORTED_FOR_DATASET_TYPE = 1021; UNSUPPORT_DATESET_TYPE? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/ITransactionSubsystem.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/ITransactionSubsystem.java: Line 28: public static final boolean IS_PROFILE_MODE = false;//true Why does this flag lives in this interface? Since it is final, there seems no need to say "IS_"? Line 30: public ILogManager getLogManager(); Remove "public" for each method? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/Resource.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/Resource.java: Line 76: LocalResource resource) throws HyracksDataException; format? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/ResourceFactory.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/ResourceFactory.java: Line 41: ILSMIOOperationCallbackFactory ioOpCallbackFactory, format? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/utils/TransactionUtil.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/utils/TransactionUtil.java: Line 31: private TransactionUtil() { add a new line before the method? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties File asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties: Line 54: 1013 = Not implemented for dataset of type %1$s The error message needs to be specific. What is "Not implemented"? Line 55: 1014 = Not implemented for index of type %1$s The error message needs to be specific. What is "Not implemented"? Line 58: 1017 = Not supported for index of type %1$s "Not supported for index of type" --> "unsupported index for type"? Line 59: 1018 = Expected secondary index A secondary index is expected, but ... The message needs to be more informative. Line 60: 1019 = Not supported for the index %1$s A secondary index is expected, but ... The message needs to be more informative. Line 61: 1020 = Index of type %1$s is not supported for datasets with composite primary keys add dataset name. Line 62: 1021 = Not supported for dataset of type %1$s Needs to be more specific. What is "not supported"? Line 63: 1022 = The filter field \"%1$s\" cannot be nullable nullable is not really understandable by an end user. The filter field \"%1$s\" cannot be an optional field. https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/AbstractExternalDatasetIndexesOperatorDescriptor.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/AbstractExternalDatasetIndexesOperatorDescriptor.java: Line 53: List<IIndexDataflowHelperFactory> treeIndexesDataflowHelperFactories, format? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/ExternalBTreeSearchOperatorDescriptor.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/ExternalBTreeSearchOperatorDescriptor.java: Line 49: IPageManagerFactory iPageManagerFactory) { iPageManagerFactory -> pageManagerFactory https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/ExternalDatasetIndexesAbortOperatorDescriptor.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/ExternalDatasetIndexesAbortOperatorDescriptor.java: Line 37: List<IIndexDataflowHelperFactory> treeIndexesDataflowHelperFactories, format? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/ExternalDatasetIndexesCommitOperatorDescriptor.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/ExternalDatasetIndexesCommitOperatorDescriptor.java: Line 42: IndexInfoOperatorDescriptor fileIndexesInfo, format? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/ExternalDatasetIndexesRecoverOperatorDescriptor.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/ExternalDatasetIndexesRecoverOperatorDescriptor.java: Line 37: List<IIndexDataflowHelperFactory> treeIndexesDataflowHelperFactories, treeIndex -> index? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/ExternalLookupOperatorDescriptor.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/ExternalLookupOperatorDescriptor.java: Line 57: IPageManagerFactory iPageManagerFactory) { iPageManagerFactory -> pageManagerFactory https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/ExternalRTreeSearchOperatorDescriptor.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/operators/ExternalRTreeSearchOperatorDescriptor.java: Line 48: IPageManagerFactory iPageManagerFactory) { iPageManagerFactory -> pageManagerFactory https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-metadata/pom.xml File asterixdb/asterix-metadata/pom.xml: Line 185: <artifactId>hadoop-hdfs</artifactId> Why metadata depend on hadoop? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/IStorageComponentProvider.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/IStorageComponentProvider.java: Line 32: * Responsible for storage components "storage components" --> "maintaining storage components" Line 35: add a get-prefix to each method? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/BTreeDataflowHelperFactoryProvider.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/BTreeDataflowHelperFactoryProvider.java: Line 61: ILSMMergePolicyFactory mergePolicyFactory, format? https://asterix-gerrit.ics.uci.edu/#/c/1451/5/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: Line 263: public void drop(MetadataProvider metadataProvider, MutableObject<MetadataTransactionContext> mdTxnCtx, documentation? Line 361: * WS Line 366: * @param mergePolicyFactory Complete the documentation. Line 402: * @throws AlgebricksException Complete documentation. Line 427: * @throws AlgebricksException Complete documentation. Line 438: * @param primaryKeyFields Complete documentation? Line 464: * @param primaryKeyFields Complete documentation? -- To view, visit https://asterix-gerrit.ics.uci.edu/1451 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: If86750cdb2436c713f6598e54d4aaaf23d9f7bbf Gerrit-PatchSet: 5 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <bamou...@gmail.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Steven Jacobs <sjaco...@ucr.edu> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: Yingyi Bu <buyin...@gmail.com> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
