abdullah alamoudi has posted comments on this change. Change subject: Introduce IComponentProvider ......................................................................
Patch Set 5: (55 comments) 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 Done 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 Done Line 259: dataset.searchCallbackFactory(metadataProvider.getStorageComponentProvider(), secondaryIndex, > searchCallBackFactory -> getSearchCallBackFactory Done 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? Done 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? Done 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? Done 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? Done 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? Done 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. (Y) will do it in a subsequent change. we need to move somethings here and there... Line 193: txnProperties); > format? Done 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? Done 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? Done 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? Done 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? Done 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 Done 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 Done 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. I think we should switch to putting 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. Done 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. Done 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.... I agree with you. This was needed though because GlobalRecoveryManager needed some components that were created after the initialization of IApplicationContextInfo 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 Done 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 Done 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? they were used when I introduced them but while refactoring, I removed their usage. 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. Done Line 73: public static final int COMPILATION_NOT_SUPPORTED_FOR_INDEX_TYPE = 1017; > NOT_SUPPORTED_FOR_INDEX_TYPE -> Done Line 74: public static final int COMPILATION_SECONDARY_INDEX_EXPECTED = 1018; > What SECONDARY_INDEX_EXPECTED mean? Done Line 75: public static final int COMPILATION_NOT_SUPPORTED_FOR_INDEX_NAME = 1019; > NOT_SUPPORTED_FOR_INDEX_NAME --> UNSUPPORTED_INDEX_NAME Done Line 77: public static final int COMPILATION_NOT_SUPPORTED_FOR_DATASET_TYPE = 1021; > UNSUPPORT_DATESET_TYPE? Done 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? Done Line 30: public ILogManager getLogManager(); > Remove "public" for each method? Done 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? Done 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? Done 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? Done 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. Done Line 55: 1014 = Not implemented for index of type %1$s > The error message needs to be specific. What is "Not implemented"? Done Line 58: 1017 = Not supported for index of type %1$s > "Not supported for index of type" --> "unsupported index for type"? Done Line 59: 1018 = Expected secondary index > A secondary index is expected, but ... Done Line 60: 1019 = Not supported for the index %1$s > A secondary index is expected, but ... Done Line 61: 1020 = Index of type %1$s is not supported for datasets with composite primary keys > add dataset name. Done Line 62: 1021 = Not supported for dataset of type %1$s > Needs to be more specific. What is "not supported"? Done Line 63: 1022 = The filter field \"%1$s\" cannot be nullable > nullable is not really understandable by an end user. Done 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? Done 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? Done 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? Done 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? because I moved many of external indexing classes to metadata. I did this in order to keep dataset operations contained within the dataset class. 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" Done Line 35: > add a get-prefix to each method? Done 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? Done Line 361: * > WS Done Line 361: * > MAJOR SonarQube violation: Done Line 366: * @param mergePolicyFactory > Complete the documentation. Done Line 402: * @throws AlgebricksException > Complete documentation. Done Line 427: * @throws AlgebricksException > Complete documentation. Done Line 438: * @param primaryKeyFields > Complete documentation? Done Line 464: * @param primaryKeyFields > Complete documentation? Done -- 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 <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Steven Jacobs <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
