You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/02/16 20:55:12 UTC
[GitHub] [pulsar] david-streamlio opened a new pull request #6335:
Refactored JCloud Tiered Storage
david-streamlio opened a new pull request #6335: Refactored JCloud Tiered Storage
URL: https://github.com/apache/pulsar/pull/6335
Motivation
In order to facilitate the support of additional JClouds-supported providers, we first needed to clean up the existing code, as there were a lot of if/then/else constructs throughout the code that were based on the assumption that we either supported AWS S3 or Google Cloud Storage. I didn't want to keep adding else if's to these code blocks for every new provider we add, so I decided to refactor the code to make it a bit cleaner
Modifications
in addition to being home for most of the aforementioned if/then/else blocks, the BlobStoreManagedLedgerOffloader class had multiple responsibilities in addition to providing an implementation for the LedgerOffloader interface. My goal was to simplify this class such that its only responsibility was to implement the LedgerOffloader interface.
The other major change was the addition of the JCloudBlobStoreProvider enum, which implements 3 interfaces that allow for it to handle the provider specific logic for things such as acquiring the credentials, validating the configuration, and creating a provider-specific instance of BlobStore.
Result
After this change, we will be able to easily add support for additional JClouds-supported providers by simply adding new elements to the JCloudBlobStoreProvider Enums since the other logic has been isolated and is not vendor specific.
See https://github.com/apache/pulsar/pull/2865 for more details
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] gaoran10 commented on a change in pull request #6335:
Refactored JCloud Tiered Storage
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #6335: Refactored JCloud Tiered Storage
URL: https://github.com/apache/pulsar/pull/6335#discussion_r407179956
##########
File path: tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreManagedLedgerOffloader.java
##########
@@ -589,26 +299,17 @@ BlobStore getReadBlobStore(Map<String, String> offloadDriverMetadata) {
return promise;
}
- public interface VersionCheck {
- void check(String key, Blob blob) throws IOException;
- }
-
+
@Override
public OffloadPolicies getOffloadPolicies() {
- return offloadPolicies;
+ // TODO Auto-generated method stub
+ return null;
}
Review comment:
Currently, when the `LedgerOffloader` is generated by the `PulsarService`, the `PulsarService` will check the namespace's `OffloadPolicies`, if the new `OffloadPolicies` is same as the existing `OffloadPolicies`, the `PulsarService` will reuse the existing `LedgerOffloader`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] david-streamlio commented on a change in pull request
#6335: Refactored JCloud Tiered Storage
Posted by GitBox <gi...@apache.org>.
david-streamlio commented on a change in pull request #6335: Refactored JCloud Tiered Storage
URL: https://github.com/apache/pulsar/pull/6335#discussion_r407537535
##########
File path: tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreManagedLedgerOffloader.java
##########
@@ -589,26 +299,17 @@ BlobStore getReadBlobStore(Map<String, String> offloadDriverMetadata) {
return promise;
}
- public interface VersionCheck {
- void check(String key, Blob blob) throws IOException;
- }
-
+
@Override
public OffloadPolicies getOffloadPolicies() {
- return offloadPolicies;
+ // TODO Auto-generated method stub
+ return null;
}
Review comment:
The focus of this PR is to refactor the tiered storage to use JCloud. I feel that we need to keep this PR to just that. I suggest we create an issue for this and assign it to me. I will make the change after this PR has been successfully merged.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] gaoran10 commented on a change in pull request #6335:
Refactored JCloud Tiered Storage
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #6335: Refactored JCloud Tiered Storage
URL: https://github.com/apache/pulsar/pull/6335#discussion_r407179956
##########
File path: tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreManagedLedgerOffloader.java
##########
@@ -589,26 +299,17 @@ BlobStore getReadBlobStore(Map<String, String> offloadDriverMetadata) {
return promise;
}
- public interface VersionCheck {
- void check(String key, Blob blob) throws IOException;
- }
-
+
@Override
public OffloadPolicies getOffloadPolicies() {
- return offloadPolicies;
+ // TODO Auto-generated method stub
+ return null;
}
Review comment:
Currently, when the `LedgerOffloader` is generated by the `PulsarService`, the `PulsarService` will check the namespace's `OffloadPolicies`, if the new `OffloadPolicies` is same as the existing `OffloadPolicies`, the `PulsarService` will reuse the existing `LedgerOffloader`, so we should provide the `OffloadPolicies` of the `LedgerOffloader`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] gaoran10 commented on a change in pull request #6335:
Refactored JCloud Tiered Storage
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #6335: Refactored JCloud Tiered Storage
URL: https://github.com/apache/pulsar/pull/6335#discussion_r407178941
##########
File path: tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/JCloudLedgerOffloaderFactory.java
##########
@@ -38,13 +40,14 @@ public static JCloudLedgerOffloaderFactory of() {
@Override
public boolean isDriverSupported(String driverName) {
- return BlobStoreManagedLedgerOffloader.driverSupported(driverName);
+ return JCloudBlobStoreProvider.driverSupported(driverName);
}
@Override
- public BlobStoreManagedLedgerOffloader create(OffloadPolicies offloadPolicies,
- Map<String, String> userMetadata,
- OrderedScheduler scheduler) throws IOException {
- return BlobStoreManagedLedgerOffloader.create(offloadPolicies, userMetadata, scheduler);
+ public BlobStoreManagedLedgerOffloader create(OffloadPolicies offloadPolicies, Map<String, String> userMetadata,
+ OrderedScheduler scheduler) throws IOException {
+
+ TieredStorageConfiguration config = TieredStorageConfiguration.create(userMetadata);
+ return BlobStoreManagedLedgerOffloader.create(config, userMetadata, scheduler);
Review comment:
Currently, the Pulsar support independent offload policy for each namespace, the `userMetadata` param contains the global offload configs and the `offloadPolicies` param contains the Namespace level offload configs. If the namespace has its own offload policy, we should use the independent offload policy, otherwise, we should use the global offload policy.
The `OffloadPolicies` has independent config for S3 and GCS, such as `s3ManagedLedgerOffloadRegion` and `gcsManagedLedgerOffloadRegion`, follow your way, we could combine them as the `region`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] david-streamlio commented on issue #6335: Refactored
JCloud Tiered Storage
Posted by GitBox <gi...@apache.org>.
david-streamlio commented on issue #6335: Refactored JCloud Tiered Storage
URL: https://github.com/apache/pulsar/pull/6335#issuecomment-609923273
Any update on this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] sijie commented on issue #6335: Refactored JCloud Tiered
Storage
Posted by GitBox <gi...@apache.org>.
sijie commented on issue #6335: Refactored JCloud Tiered Storage
URL: https://github.com/apache/pulsar/pull/6335#issuecomment-609949408
@david-streamlio sorry for the late response. @jiazhai @codelipenghui and @gaoran10 can you guys review this pull request? @david-streamlio is waiting for a review.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] david-streamlio commented on a change in pull request
#6335: Refactored JCloud Tiered Storage
Posted by GitBox <gi...@apache.org>.
david-streamlio commented on a change in pull request #6335: Refactored JCloud Tiered Storage
URL: https://github.com/apache/pulsar/pull/6335#discussion_r407537446
##########
File path: tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/JCloudLedgerOffloaderFactory.java
##########
@@ -38,13 +40,14 @@ public static JCloudLedgerOffloaderFactory of() {
@Override
public boolean isDriverSupported(String driverName) {
- return BlobStoreManagedLedgerOffloader.driverSupported(driverName);
+ return JCloudBlobStoreProvider.driverSupported(driverName);
}
@Override
- public BlobStoreManagedLedgerOffloader create(OffloadPolicies offloadPolicies,
- Map<String, String> userMetadata,
- OrderedScheduler scheduler) throws IOException {
- return BlobStoreManagedLedgerOffloader.create(offloadPolicies, userMetadata, scheduler);
+ public BlobStoreManagedLedgerOffloader create(OffloadPolicies offloadPolicies, Map<String, String> userMetadata,
+ OrderedScheduler scheduler) throws IOException {
+
+ TieredStorageConfiguration config = TieredStorageConfiguration.create(userMetadata);
+ return BlobStoreManagedLedgerOffloader.create(config, userMetadata, scheduler);
Review comment:
The focus of this PR is to refactor the tiered storage to use JCloud. I feel that we need to keep this PR to just that. I suggest we create an issue for this and assign it to me. I will make the change after this PR has been successfully merged.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services