david-streamlio commented on a change in pull request #2865: Refactored JCloud Tiered Storage URL: https://github.com/apache/pulsar/pull/2865#discussion_r232363051
########## File path: tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/provider/BlobStoreRepository.java ########## @@ -0,0 +1,85 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.bookkeeper.mledger.offload.jcloud.provider; + +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; + +import org.jclouds.blobstore.BlobStore; + +/** + * Centralized storage location for all the Blobstore instances + * created and used for tiered-storage. + * <p> + * Users should first check to see if an existing Blobstore for the + * the given BlobStoreKey exists, and if so, use the existing one. + * Otherwise a new one will be created and added to the Repository + * </p> + */ +public class BlobStoreRepository { Review comment: I think passing in a map into the constructor is the best approach, as it insulates from a lot of future changes, and allows us to add non-blob-storage JClouds providers in the future, such as [Filesystem](https://jclouds.apache.org/guides/filesystem/) and [Databases](https://jclouds.apache.org/guides/jdbc/) That is why I chose to use the TieredStorageConfiguration object to encapsulate all of these properties that get passed in to the constructor. This object also, then delegates to responsibility of instantiating the BlobStore to the JCloudBlobStoreProvider Enumeration object (which is defined in the BlobStoreBuilder interface that all Enums have to implement). I feel this is a more IoC approach. As for the placement of the data structure that holds the BlobStore instances, I guess it doesn't really matter if it is a member field of the BlobStoreManagedLedgerOffloader or not. I can move it there if you like, but my thought process was that by de-coupling the storage of the BlobStores from the offloader gives us the opportunity to re-use them across multiple instances of offloaders in the future, if we were to have multiple providers or even a pool of offloaders running concurrently, as some of these remote connections might be slow and/or timeout, so it might be best to have multiple instances running. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
