ivankelly commented on a change in pull request #2865: Refactored JCloud Tiered Storage URL: https://github.com/apache/pulsar/pull/2865#discussion_r234760830
########## 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: > 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. If we want to share like this, then the container should be passed in as an argument to the offloader. However, I don't think it'll be the case where we have multiple instances of the same offloader, at least not in the near-term, so I would suggest you move it into the scope of the BlobStoreManagedLedgerOffloader object, as blobstores was previously. ---------------------------------------------------------------- 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: us...@infra.apache.org With regards, Apache Git Services
