Yingyi Bu has posted comments on this change. Change subject: Introduce IStorageComponentProvider ......................................................................
Patch Set 6: (37 comments) https://asterix-gerrit.ics.uci.edu/#/c/1451/6/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cc/IStorageExtension.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cc/IStorageExtension.java: Line 24: public interface IStorageExtension extends IExtension { I couldn't find a class that implements IStorageExtension? https://asterix-gerrit.ics.uci.edu/#/c/1451/6/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 37: * @return transactionSubsystemProvider @return a {@link package.class} instance, which .... */ Line 38: * @throws AlgebricksException document when an exception is supposed to be thrown. Line 43: * @return ioOperationSchedulerProvider @return a {@link package.class} instance, which customizes ... */ Line 44: * @throws AlgebricksException document when an exception is supposed to be thrown. Line 49: * @return indexLifecycleManagerProvider @return a {@link package.class} instance, which ..... */ Line 50: * @throws AlgebricksException document when an exception is supposed be thrown. Line 55: * @return runtimeComponentProvider @return a {@link package.class} instance, which ..... */ Line 56: * @throws AlgebricksException document when an exception is supposed to be thrown. Line 58: IRuntimeComponentProvider runtimeComponentProvider() throws AlgebricksException; The name IRuntimeComponentProvider is a bit confusing. I think it'd be more clear to call it IStorageManager. Line 61: * @return metadataPageManagerFactory @return a {@link package.class} instance, which ..... */ Line 68: IPrimitiveValueProviderFactory primitiveValueProviderFactory(); @return a {@link package.class} instance, which ..... */ Line 73: IBinaryComparatorFactoryProvider comparatorFactoryProvider(); @return a {@link package.class} instance, which ..... */ Line 78: TypeTraitProvider typeTraitProvider(); @return a {@link package.class} instance, which ..... */ https://asterix-gerrit.ics.uci.edu/#/c/1451/6/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/bootstrap/MetadataBootstrap.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/bootstrap/MetadataBootstrap.java: Line 146: public static void startUniverse(IStorageComponentProvider storageManager, Would it make sense to put storage manager and properties provider into ncApplicationContext? In this way, we do not need to constantly change the method surface. Line 337: IStorageComponentProvider storageComponentProvider, IMetadataIndex index) throws HyracksDataException { Would it make sense to make storage component provider part of INCApplicationContext, so that we do not need to constantly change the method surface. https://asterix-gerrit.ics.uci.edu/#/c/1451/6/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/MetadataProvider.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/MetadataProvider.java: Line 156: private final IStorageComponentProvider storageComponentProvider; Add transactionSystemProvider, metadataFactory, etc, each as a private field in MetadataProvider, so that you can avoid too many provider.getXXX().getXXX() calls? Assume those factories seem not changing over time during the life time of an instance. Line 940: public Pair<IFileSplitProvider, AlgebricksPartitionConstraint> splitAndConstraints( splitAndConstraints --> splitProviderAndPartitionConstraints The old name looks clear to me. We're not returning splits, but SplitProvider. Also, PartitionConstraint is clearer than constraints. Line 946: public Pair<IFileSplitProvider, AlgebricksPartitionConstraint> splitAndConstraints( splitAndConstraints --> splitProviderAndPartitionConstraints(...) Line 1095: jobId.getId(); remove the useless jobId.getId() ? https://asterix-gerrit.ics.uci.edu/#/c/1451/6/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 101: BTreeDataflowHelperFactoryProvider.INSTANCE; How hard is it to avoid singletons here? Make constructors much more complex? Line 273: "Can't drop dataset since it is connected to active entity: " + listener.getEntityId()); Use error code? https://asterix-gerrit.ics.uci.edu/#/c/1451/6/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entitytupletranslators/IndexTupleTranslator.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entitytupletranslators/IndexTupleTranslator.java: Line 125: List<List<String>> searchKey = new ArrayList<>(); > MAJOR SonarQube violation: fix this? Line 141: List<IAType> searchKeyType = new ArrayList<>(searchKey.size()); > MAJOR SonarQube violation: fix this? https://asterix-gerrit.ics.uci.edu/#/c/1451/6/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/feeds/FeedOperations.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/feeds/FeedOperations.java: Line 64: private FeedOperations() { Add a new line before the constructor? https://asterix-gerrit.ics.uci.edu/#/c/1451/6/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/ExternalIndexingOperations.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/ExternalIndexingOperations.java: Line 296: if (fileSystemFile.getSize() == metadataFile.getSize()) { > MAJOR SonarQube violation: this can be easily fixed by flip the first "if" in the while loop? https://asterix-gerrit.ics.uci.edu/#/c/1451/6/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: Line 57: ARecordType metaType, format? https://asterix-gerrit.ics.uci.edu/#/c/1451/6/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/RTreeDataflowHelperFactoryProvider.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/RTreeDataflowHelperFactoryProvider.java: Line 62: public IIndexDataflowHelperFactory indexDataflowHelperFactory( getIndexDataflowHelperFactory? Line 141: numNestedSecondaryKeyFields Could this be moved to datasets? It seems they're dataset-driven decisions? https://asterix-gerrit.ics.uci.edu/#/c/1451/6/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/transaction/AsterixRuntimeComponentsProvider.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/transaction/AsterixRuntimeComponentsProvider.java: Line 37: It seems no body use this class. Remove it? https://asterix-gerrit.ics.uci.edu/#/c/1451/6/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/transaction/TransactionContext.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/transaction/TransactionContext.java: Line 102: public TransactionContext(JobId jobId, ITransactionSubsystem transactionSubsystem) throws ACIDException { > MAJOR SonarQube violation: Fix this? https://asterix-gerrit.ics.uci.edu/#/c/1451/6/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/freepage/AppendOnlyLinkedMetadataPageManagerFactory.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/freepage/AppendOnlyLinkedMetadataPageManagerFactory.java: Line 30: private AppendOnlyLinkedMetadataPageManagerFactory() { format & new line. https://asterix-gerrit.ics.uci.edu/#/c/1451/6/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMOperationTrackerProvider.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMOperationTrackerProvider.java: Line 26: public interface ILSMOperationTrackerProvider extends Serializable { ILSMOperationTrackerProvider --> ILSMOperationTrackerFactory? IMO, it's a factory. Here is our convention: a provider is a factory of factories. https://asterix-gerrit.ics.uci.edu/#/c/1451/6/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/NoOpOperationTrackerProvider.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/NoOpOperationTrackerProvider.java: Line 36: public class NoOpOperationTrackerProvider implements ILSMOperationTrackerProvider { NoOpOperationTrackerProvider --> NoOpOperationTrackerFactory? https://asterix-gerrit.ics.uci.edu/#/c/1451/6/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/IRuntimeComponentProvider.java File hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/IRuntimeComponentProvider.java: Line 29: public interface IRuntimeComponentProvider extends Serializable { The name still seems confusing. It's all storage related stuff. RuntimeComponentProvider is too general. IStorageManager makes sense to me. Line 30: public IBufferCache getBufferCache(IHyracksTaskContext ctx); Document each individual interface method. https://asterix-gerrit.ics.uci.edu/#/c/1451/6/hyracks-fullstack/hyracks/hyracks-test-support/src/main/java/org/apache/hyracks/test/support/TestStorageManagerComponentHolder.java File hyracks-fullstack/hyracks/hyracks-test-support/src/main/java/org/apache/hyracks/test/support/TestStorageManagerComponentHolder.java: Line 61: AppendOnlyLinkedMetadataPageManagerFactory.INSTANCE; Is it hard to avoid the singleton? -- 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: 6 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
