Namespace level offloader by gaoran10 · Pull Request #6183 · apache/pulsar

gaoran10 · 2020-01-31T21:05:38Z

This work was originally developed by @KannarFr at pull request #4739. This pull request was based on #4739

Motivation

Currently, the offload operation only have the cluster level configuration, can't set the offload configuration at the namespace level, it's inflexible.

Modifications

Add the namespace offload policies.

Dependencies (does it add or upgrade a dependency): (no)
The public API: (yes)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (yes)
The admin cli options: (yes)
Anything that affects deployment: (don't know)

Documentation

Does this pull request introduce a new feature? (yes)
If yes, how is the feature documented? (docs)

…namespace-level-offloader # Conflicts: # pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Namespaces.java # pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java

codelipenghui

The main concern is the offloader cache, because is users frequently modified the policy, there will be some invalid Offloader objects here.

codelipenghui · 2020-02-01T02:27:46Z

                }
            } else {
-                LOG.info("No ledger offloader configured, using NULL instance");
+                LOG.warn("No ledger offloader configured, using NULL instance");


Keep log level to info, we don't need to warn users when they have not enable the offloader.

codelipenghui · 2020-02-01T02:37:19Z

+import org.apache.pulsar.common.policies.data.OffloadPolicies;
+import org.apache.pulsar.common.policies.data.OffloadPolicies;


Looks duplicated

codelipenghui · 2020-02-01T02:57:16Z

    }

-    private LedgerOffloader initManagedLedgerOffloader(PulsarConnectorConfig conf) {
+    private LedgerOffloader initManagedLedgerOffloader(PulsarConnectorConfig conf, OffloadPolicies offloadPolicies) {


Same as PulsarService, use initManagedLedgerOffloader(OffloadPolicies offloadPolicies). For default offloader, just convert serverConfiguration to the OffloadPolicies.

2. Add methods for interface LedgerOffloader, add the getOffloadPolicies method, add close method; 2. Unified the offload policies configuration type, use the OffloadPolicies instead of TieredStorageConfigurationData and FileSystemConfigurationData.

codelipenghui

Overall looks good, please help try to add some integration test to cover the changes.
I'm concern about current implementation related to namespace policy changes in Pulsar SQL.
In this PR, split manager fetches namespace policies. This will lead to more admin API requests.

@sijie @jiazhai Is there any better approach? or we need to add a flag to disable fetch namespace policy.
Pulsar SQL may have many workers, regularly fetch still not a good way.

2. modify the `get-offload` to `get-offload-policies`; 3. add the OffloadPolicies validate in NamespacesBase.

sijie

overall looks good.

@codelipenghui I think split manager is only run when a job is submitted. once the split is created, I don't think each split is fetching the namespace policies. so that should be fine.

sijie · 2020-02-07T00:54:50Z

@gaoran10 well done!

…loader

codelipenghui · 2020-02-07T02:11:50Z

@gaoran10 Please take a look sijie's comments. nice job 👍

gaoran10 · 2020-02-07T11:42:04Z

@gaoran10 Please take a look sijie's comments. nice job 👍

@codelipenghui ok, I will fix them.

gaoran10 · 2020-02-07T11:42:34Z

@sijie Thank you!

2. modify setOffloadPolicies api error message.

…10/pulsar into namespace-level-offloader

sijie · 2020-02-10T08:07:48Z

Thanks @KannarFr for the initial change and thanks @gaoran10 for continuing @KannarFr 's awesome work. It is a great example of teamwork and community collaboration! Thanks everyone involved in this pull request.

KannarFr · 2020-02-10T13:22:17Z

@gaoran10 Hi, thanks for the work you did there.

We are missing a point that I didn't precise in original issue: internalSetOffloadDeletionLag and internalSetOffloadThreshold should have their own values per ns or use default configuration. I created a new issue #6283. (i.e. currently we will have the same trigger policies for every ns, but we want to define per ns).

gaoran10 · 2020-02-10T16:09:23Z

We are missing a point that I didn't precise in original issue: internalSetOffloadDeletionLag and internalSetOffloadThreshold should have their own values per ns or use default configuration. I created a new issue #6283. (i.e. currently we will have the same trigger policies for every ns, but we want to define per ns).

@KannarFr Hi, thanks for your suggestions. In currently, maybe the config internalSetOffloadDeletionLag and internalSetOffloadThreshold belong to namespace is supported, refer to these API 'http://pulsar.apache.org/admin-rest-api/?version=2.5.0#operation/setOffloadDeletionLag' and 'http://pulsar.apache.org/admin-rest-api/?version=2.5.0#operation/setOffloadThreshold'.

KannarFr · 2020-02-10T16:38:37Z

Yup, closed on #6283 but forget here, sorry.

tuteng · 2020-03-21T02:59:06Z

Add label to 2.5.1, due to #6325 dependency

### Motivation Currently, the offload operation only have the cluster level configuration, can't set the offload configuration at the namespace level, it's inflexible. ### Modifications Add the namespace offload policies. (cherry picked from commit fd03be5)

### Motivation Currently, the offload operation only have the cluster level configuration, can't set the offload configuration at the namespace level, it's inflexible. ### Modifications Add the namespace offload policies.

gao.ran added 5 commits February 1, 2020 04:12

add namespace offload policies

a95fddb

fix

72a76cb

add namespace offload policies

e5caac0

fix

f23cd79

Merge remote-tracking branch 'origin/namespace-level-offloader' into …

02215a9

…namespace-level-offloader # Conflicts: # pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Namespaces.java # pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java

gaoran10 mentioned this pull request Jan 31, 2020

offload bucket per namespace #4739

Closed

sijie assigned gaoran10 Jan 31, 2020

sijie requested review from codelipenghui, jiazhai and merlimat January 31, 2020 23:06

codelipenghui requested changes Feb 1, 2020

View reviewed changes

codelipenghui added this to the 2.6.0 milestone Feb 1, 2020

codelipenghui added the area/tieredstorage label Feb 1, 2020

gao.ran added 2 commits February 5, 2020 14:05

add licensed header for CredentialsUtil

9874009

codelipenghui reviewed Feb 5, 2020

View reviewed changes

gao.ran added 2 commits February 5, 2020 22:29

1. modify the set-offload to set-offload-policies;

a898f24

2. modify the `get-offload` to `get-offload-policies`; 3. add the OffloadPolicies validate in NamespacesBase.

checkstyle fix

0a4d81e

sijie approved these changes Feb 7, 2020

View reviewed changes

Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java Outdated

Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Namespaces.java Outdated

Merge remote-tracking branch 'apache/master' into namespace-level-off…

b45afe9

…loader

gaoran10 closed this Feb 7, 2020

gaoran10 reopened this Feb 7, 2020

gao.ran added 3 commits February 8, 2020 22:59

1. modify internalSetOffloadPolicies method to asynchronous;

4300e22

2. modify setOffloadPolicies api error message.

Merge branch 'namespace-level-offloader' of https://github.com/gaoran…

1b4e55d

…10/pulsar into namespace-level-offloader

exception process fix

9fc437e

codelipenghui approved these changes Feb 10, 2020

View reviewed changes

fix unit test

998fcb5

codelipenghui merged commit fd03be5 into apache:master Feb 10, 2020

tuteng added the release/2.5.1 label Mar 21, 2020

tuteng mentioned this pull request Mar 21, 2020

Allow to enable/disable delayed delivery for messages on namespace #5915

Merged

wolfstudy mentioned this pull request Jun 10, 2020

Add offload-policies commands to pulsarctl streamnative/pulsarctl#228

Open

poorbarcode mentioned this pull request Sep 3, 2025

[fix][broker]Fix dirty reading of namespace level offload thresholds #24696

Merged

4 tasks

		import org.apache.pulsar.common.policies.data.OffloadPolicies;
		import org.apache.pulsar.common.policies.data.OffloadPolicies;

Uh oh!

Conversation

gaoran10 commented Jan 31, 2020 • edited by sijie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Documentation

Uh oh!

codelipenghui left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codelipenghui Feb 1, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codelipenghui Feb 1, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codelipenghui Feb 1, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codelipenghui left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sijie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sijie commented Feb 7, 2020

Uh oh!

codelipenghui commented Feb 7, 2020

Uh oh!

gaoran10 commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaoran10 commented Feb 7, 2020

Uh oh!

sijie commented Feb 10, 2020

Uh oh!

KannarFr commented Feb 10, 2020

Uh oh!

gaoran10 commented Feb 10, 2020

Uh oh!

KannarFr commented Feb 10, 2020

Uh oh!

tuteng commented Mar 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gaoran10 commented Jan 31, 2020 •

edited by sijie

Loading

codelipenghui left a comment •

edited

Loading

codelipenghui left a comment •

edited

Loading

gaoran10 commented Feb 7, 2020 •

edited

Loading