Refactored JCloud Tiered Storage #6335

david-streamlio · 2020-02-16T20:55:11Z

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 #2865 for more details

david-streamlio · 2020-04-06T17:13:43Z

Any update on this?

sijie · 2020-04-06T18:03:28Z

@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.

gaoran10 · 2020-04-12T10:32:53Z

...src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/JCloudLedgerOffloaderFactory.java

+            OrderedScheduler scheduler) throws IOException {
+
+        TieredStorageConfiguration config = TieredStorageConfiguration.create(userMetadata);
+        return BlobStoreManagedLedgerOffloader.create(config, userMetadata, scheduler);


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.

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.

Sorry for the late response. If we don't care about the OffloadPolicies config, it will cause the namespace offload policies invalid.

gaoran10 · 2020-04-12T10:42:47Z

.../java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreManagedLedgerOffloader.java

    public OffloadPolicies getOffloadPolicies() {
-        return offloadPolicies;
+        // TODO Auto-generated method stub
+        return null;
    }


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.

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.

codelipenghui · 2020-06-04T05:19:17Z

move to 2.7.0 first.

david-streamlio · 2020-06-06T18:26:56Z

Anything needed from me on this?

sijie · 2020-06-10T05:06:30Z

@gaoran10 @codelipenghui can you review this change again?

jiazhai · 2020-09-08T00:57:23Z

@david-streamlio Would you please help resolve the conflicts with master branch?

david-streamlio · 2020-09-08T17:40:34Z

I will work on this conflicts shortly.

jiazhai · 2020-09-09T03:43:48Z

@congbobo184 @gaoran10 Would you please help review this PR?

gaoran10 · 2020-09-21T04:28:56Z

LGTM, that's so cool.

I added an issue to track the problem with the namespace level offload feature, we could complete this PR first and repair that problem. @david-streamlio Hi, could you make a rebase for this PR? Thanks.

david-streamlio · 2020-09-21T16:48:27Z

Code has been rebased.

gaoran10 · 2020-09-22T01:54:07Z

/pulsarbot run-failure-checks

gaoran10 · 2020-10-12T02:26:16Z

@david-streamlio Hi, there are still some problems in this PR, I made some changes based on your work, could you have a look and add these changes to this PR, thanks. gaoran10#12

Signed-off-by: xiaolong.ran <rxl@apache.org>

wolfstudy · 2020-10-18T08:12:56Z

/pulsarbot run-failure-checks

jiazhai · 2020-10-19T01:55:12Z

Would like to merge this first.

wolfstudy · 2020-10-29T08:27:34Z

Move this change to 2.6.2, because the #8310 depends on it.

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 #2865 for more details * Refactored JCloud Tiered Storage * Refactored JCloud Tiered Storage * Added missing import statements * Refactored JCloud Tiered Storage * Refactored JCloud Tiered Storage * Added missing import statements * Refactored JCloud Tiered Storage * Refactored JCloud Tiered Storage * Added missing import statements * fix test * add test logs * fix logs * fix test * fix * fix * fix broker log * fix configuration * fix * fix * fix * fix get BlobStore * repair test presto query tiered storage data * fix test * fix test * fix test TestPrestoQueryTieredStorage * fix * fix Co-authored-by: Sijie Guo <sijie@apache.org> Co-authored-by: gaoran10 <gaoran_10@126.com> Co-authored-by: xiaolong.ran <rxl@apache.org> (cherry picked from commit d3a5233)

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 apache#2865 for more details * Refactored JCloud Tiered Storage * Refactored JCloud Tiered Storage * Added missing import statements * Refactored JCloud Tiered Storage * Refactored JCloud Tiered Storage * Added missing import statements * Refactored JCloud Tiered Storage * Refactored JCloud Tiered Storage * Added missing import statements * fix test * add test logs * fix logs * fix test * fix * fix * fix broker log * fix configuration * fix * fix * fix * fix get BlobStore * repair test presto query tiered storage data * fix test * fix test * fix test TestPrestoQueryTieredStorage * fix * fix Co-authored-by: Sijie Guo <sijie@apache.org> Co-authored-by: gaoran10 <gaoran_10@126.com> Co-authored-by: xiaolong.ran <rxl@apache.org>

# Motivation The PR #6335 lost some PR changes, related PRs as below. 1. PR 4196 (2019/5/29 Merli) Configure static PulsarByteBufAllocator to handle OOM errors (#4196) 2. PR 5356 (2019/10/30 Kelly) [TIEREDSTORAGE] Only seek when reading unexpected entry (#5356) 3. PR 4433 (2019/6/4 Higham) [tiered-storage] Add support for AWS instance and role creds (#4433)

…#6335

# Motivation The PR #6335 lost some PR changes, related PRs as below. 1. PR 4196 (2019/5/29 Merli) Configure static PulsarByteBufAllocator to handle OOM errors (#4196) 2. PR 5356 (2019/10/30 Kelly) [TIEREDSTORAGE] Only seek when reading unexpected entry (#5356) 3. PR 4433 (2019/6/4 Higham) [tiered-storage] Add support for AWS instance and role creds (#4433) (cherry picked from commit 68759ff)

…ache#8630) # Motivation The PR apache#6335 lost some PR changes, related PRs as below. 1. PR 4196 (2019/5/29 Merli) Configure static PulsarByteBufAllocator to handle OOM errors (#4196) 2. PR 5356 (2019/10/30 Kelly) [TIEREDSTORAGE] Only seek when reading unexpected entry (#5356) 3. PR 4433 (2019/6/4 Higham) [tiered-storage] Add support for AWS instance and role creds (#4433) (cherry picked from commit 68759ff) (cherry picked from commit 8793963)

…ache#8630) # Motivation The PR apache#6335 lost some PR changes, related PRs as below. 1. PR 4196 (2019/5/29 Merli) Configure static PulsarByteBufAllocator to handle OOM errors (#4196) 2. PR 5356 (2019/10/30 Kelly) [TIEREDSTORAGE] Only seek when reading unexpected entry (#5356) 3. PR 4433 (2019/6/4 Higham) [tiered-storage] Add support for AWS instance and role creds (#4433) (cherry picked from commit 68759ff) (cherry picked from commit 8793963) (cherry picked from commit 0388a1a)

Refactored JCloud Tiered Storage

f557f27

david-streamlio mentioned this pull request Feb 16, 2020

Refactored JCloud Tiered Storage #2865

Closed

sijie requested review from merlimat, sijie, codelipenghui and jiazhai February 16, 2020 21:41

sijie assigned david-streamlio Feb 16, 2020

sijie added the area/tieredstorage label Feb 16, 2020

sijie added this to the 2.6.0 milestone Feb 16, 2020

gaoran10 reviewed Apr 12, 2020

View reviewed changes

codelipenghui modified the milestones: 2.6.0, 2.7.0 Jun 4, 2020

gaoran10 mentioned this pull request Sep 21, 2020

[Offload] Namespace offload policies broken streamnative/pulsar-archived#1484

Closed

David Kjerrumgaard added 3 commits September 21, 2020 09:03

Refactored JCloud Tiered Storage

fe10712

Rebased

119e348

Added missing import statements

11f4c70

Merge branch 'master' into jcloud-take2

3dec655

apache deleted a comment from gaoran10 Sep 23, 2020

gaoran10 added 10 commits October 12, 2020 00:45

fix

70f3d25

fix broker log

f6efaef

fix configuration

453307c

fix

53fb5a6

fix

b4e07c0

fix

c38acac

fix get BlobStore

654b96e

repair test presto query tiered storage data

0de001c

fix test

e28258a

fix test

f14ac64

gaoran10 and others added 4 commits October 16, 2020 11:44

fix test TestPrestoQueryTieredStorage

52857fa

fix

f019067

fix

41d44da

merge gaoran branch to david jcloud-take2

2ca1d46

Signed-off-by: xiaolong.ran <rxl@apache.org>

jiazhai approved these changes Oct 19, 2020

View reviewed changes

jiazhai merged commit d3a5233 into apache:master Oct 19, 2020

wolfstudy added the release/2.6.2 label Oct 29, 2020

gaoran10 mentioned this pull request Nov 20, 2020

[Tiered Storage] Fix merge conflicts introduced by PR #6335 #8630

Merged

github-actions bot pushed a commit that referenced this pull request Nov 21, 2020

[bot-cherry-pick][Tiered Storage] Fix merge conflicts introduced by PR …

d66a58c

…#6335

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored JCloud Tiered Storage #6335

Refactored JCloud Tiered Storage #6335

david-streamlio commented Feb 16, 2020

david-streamlio commented Apr 6, 2020

sijie commented Apr 6, 2020

gaoran10 Apr 12, 2020

david-streamlio Apr 13, 2020

gaoran10 Jul 1, 2020

gaoran10 Apr 12, 2020 •

edited

Loading

david-streamlio Apr 13, 2020

codelipenghui commented Jun 4, 2020

david-streamlio commented Jun 6, 2020

sijie commented Jun 10, 2020

jiazhai commented Sep 8, 2020

david-streamlio commented Sep 8, 2020

jiazhai commented Sep 9, 2020

gaoran10 commented Sep 21, 2020

david-streamlio commented Sep 21, 2020

gaoran10 commented Sep 22, 2020

gaoran10 commented Oct 12, 2020 •

edited

Loading

wolfstudy commented Oct 18, 2020

jiazhai commented Oct 19, 2020

wolfstudy commented Oct 29, 2020

Refactored JCloud Tiered Storage #6335

Refactored JCloud Tiered Storage #6335

Conversation

david-streamlio commented Feb 16, 2020

david-streamlio commented Apr 6, 2020

sijie commented Apr 6, 2020

gaoran10 Apr 12, 2020

Choose a reason for hiding this comment

david-streamlio Apr 13, 2020

Choose a reason for hiding this comment

gaoran10 Jul 1, 2020

Choose a reason for hiding this comment

gaoran10 Apr 12, 2020 • edited Loading

Choose a reason for hiding this comment

david-streamlio Apr 13, 2020

Choose a reason for hiding this comment

codelipenghui commented Jun 4, 2020

david-streamlio commented Jun 6, 2020

sijie commented Jun 10, 2020

jiazhai commented Sep 8, 2020

david-streamlio commented Sep 8, 2020

jiazhai commented Sep 9, 2020

gaoran10 commented Sep 21, 2020

david-streamlio commented Sep 21, 2020

gaoran10 commented Sep 22, 2020

gaoran10 commented Oct 12, 2020 • edited Loading

wolfstudy commented Oct 18, 2020

jiazhai commented Oct 19, 2020

wolfstudy commented Oct 29, 2020

gaoran10 Apr 12, 2020 •

edited

Loading

gaoran10 commented Oct 12, 2020 •

edited

Loading