abdullah alamoudi has posted comments on this change. Change subject: Introduce IStorageComponentProvider ......................................................................
Patch Set 6: (30 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? That is correct. I can remove it but let's keep it. I have a feeling that we will need it soon. 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 .... */ Done Line 38: * @throws AlgebricksException > document when an exception is supposed to be thrown. Done Line 43: * @return ioOperationSchedulerProvider > @return a {@link package.class} instance, which customizes ... */ Done Line 44: * @throws AlgebricksException > document when an exception is supposed to be thrown. Done Line 49: * @return indexLifecycleManagerProvider > @return a {@link package.class} instance, which ..... */ Done Line 50: * @throws AlgebricksException > document when an exception is supposed be thrown. Done Line 55: * @return runtimeComponentProvider > @return a {@link package.class} instance, which ..... */ Done Line 56: * @throws AlgebricksException > document when an exception is supposed to be thrown. Done Line 58: IRuntimeComponentProvider runtimeComponentProvider() throws AlgebricksException; > The name IRuntimeComponentProvider is a bit confusing. I think it'd be m Done Line 61: * @return metadataPageManagerFactory > @return a {@link package.class} instance, which ..... */ Done Line 68: IPrimitiveValueProviderFactory primitiveValueProviderFactory(); > @return a {@link package.class} instance, which ..... */ Done Line 73: IBinaryComparatorFactoryProvider comparatorFactoryProvider(); > @return a {@link package.class} instance, which ..... */ Done Line 78: TypeTraitProvider typeTraitProvider(); > @return a {@link package.class} instance, which ..... */ Done 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 ncA Done Line 337: IStorageComponentProvider storageComponentProvider, IMetadataIndex index) throws HyracksDataException { > Would it make sense to make storage component provider part of INCApplicati Done 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 compl added a todo. It is not difficult but will need to change multiple places. I think there is a better way to do it but it requires some more refactoring. Hence, I think it should be done in a subsequent change :) 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<>(); > fix this? Done Line 141: List<IAType> searchKeyType = new ArrayList<>(searchKey.size()); > fix this? Done 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? Done 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? Done 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? Done 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? Done 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 { > Fix this? Done 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. Done 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? Done 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? Done 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. Done Line 30: public IBufferCache getBufferCache(IHyracksTaskContext ctx); > Document each individual interface method. Done 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? I think it is okay to use a singleton here for two reasons: 1. it is a test class 2. AppendOnlyLinkedMetadataPageManagerFactory.INSTANCE is a stateless class. IMO, it is okay to have singletons for stateless classes and we're assigning it to a static member here. if we want to change it later, we can do that from a single location here. -- 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
