[Issue 6283][Offloaders] Offload policies per ns with deletionLag & threshold #6381

KannarFr · 2020-02-20T13:07:05Z

Fixes #6283

Modifications

Define and use custom deletionLag and threshold for offloadpolicies per ns

Verifying this change

Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as .

Does this pull request potentially affect one of the following parts:

The public API: yes, new fields to add to offloadpolicies
The admin cli options: yes, new fields to add to offloadpolicies

Documentation

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

KannarFr · 2020-02-20T13:50:45Z

@gaoran10 Hi, can you review this?

codelipenghui · 2020-02-22T06:07:58Z

@gaoran10 Please also help review this PR since you have done lots of work in the namespace level offload policy.

codelipenghui

Looks good to me, just left some minor comments, please take a look.

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedgerConfig.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

gaoran10 · 2020-02-22T06:55:51Z

pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/OffloadPolicies.java

    public final static String[] DRIVER_NAMES = {"S3", "aws-s3", "google-cloud-storage", "filesystem"};
    public final static String DEFAULT_OFFLOADER_DIRECTORY = "./offloaders";
+    public final static long DEFAULT_OFFLOAD_OFFLOAD_THRESHOLD = -1;
+    public final static Long DEFAULT_OFFLOAD_OFFLOAD_DELETION_LAG_MS = null;


Maybe the default value of this config could keep the original value (4 hours).

As it's deletion configuration I prefer set the default value to no deletion. WDYT?

gaoran10 · 2020-02-22T07:16:02Z

pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java

+            Long offloadAfterElapsedInMillis = OffloadPolicies.DEFAULT_OFFLOAD_OFFLOAD_DELETION_LAG_MS;
+            if (StringUtils.isNotEmpty(offloadAfterElapsedStr) ) {
+                Long offloadAfterElapsed = TimeUnit.SECONDS.toMillis(RelativeTimeUtil.parseRelativeTimeInSeconds(offloadAfterElapsedStr));
+                if (positiveCheck("OffloadAfterElapsed", offloadAfterElapsed)
+                        && maxValueCheck("OffloadAfterElapsed", offloadAfterElapsed, Long.MAX_VALUE)) {
+                    offloadAfterElapsedInMillis = new Long(offloadAfterElapsed);
+                }
+            }
+
+            long offloadAfterThresholdInBytes = OffloadPolicies.DEFAULT_READ_BUFFER_SIZE_IN_BYTES;
+            if (StringUtils.isNotEmpty(offloadAfterThresholdStr) ) {
+                long offloadAfterThreshold = validateSizeString(offloadAfterThresholdStr);
+                if (positiveCheck("OffloadAfterThreshold", offloadAfterThreshold)
+                        && maxValueCheck("OffloadAfterThreshold", offloadAfterThreshold, Long.MAX_VALUE)) {
+                    offloadAfterThresholdInBytes = new Long(offloadAfterThreshold);
+                }
+            }
+


I have a question, there are tow CLI-commands can change the managedLedgerOffloadTreshold config and the managedLedgerOffloadDeletionLagMs config, if we use the command set-offload-threshold or set-offload-deletion-lag to set the config first, then we use the command set-offload-policies to set the bucket except for managedLedgerOffloadTreshold and managedLedgerOffloadDeletionLagMs, they will be overridden to default.

"they will be overridden to default.", can you link where override is done?

gaoran10 · 2020-02-22T07:26:59Z

Looks good to me!

Could you fix the description of the command set-offload-policies and get-offload-policies in the doc reference-pulsar-admin.md.

KannarFr · 2020-02-24T14:13:38Z

I am not sure the failing tests are linked to this PR? Isn't it?

sijie · 2020-02-24T18:16:50Z

/pulsarbot run-failure-checks

KannarFr · 2020-02-26T01:15:13Z

After discuss with @sijie, I prefer to create a fresh PR on #6422.

KannarFr force-pushed the offload-policies-per-ns branch 2 times, most recently from fd25451 to 7299bc2 Compare February 20, 2020 13:15

KannarFr force-pushed the offload-policies-per-ns branch 6 times, most recently from 08a513a to f020923 Compare February 21, 2020 14:16

sijie assigned KannarFr Feb 22, 2020

sijie requested review from codelipenghui, jiazhai, sijie and tuteng February 22, 2020 06:04

sijie added the area/tieredstorage label Feb 22, 2020

sijie added this to the 2.6.0 milestone Feb 22, 2020

codelipenghui reviewed Feb 22, 2020

View reviewed changes

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedgerConfig.java Outdated Show resolved Hide resolved

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedgerConfig.java Outdated Show resolved Hide resolved

codelipenghui requested changes Feb 22, 2020

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java Show resolved Hide resolved

gaoran10 reviewed Feb 22, 2020

View reviewed changes

KannarFr mentioned this pull request Feb 24, 2020

Pull Request checks detected documentation changes but it's not #6409

Closed

KannarFr force-pushed the offload-policies-per-ns branch from f020923 to 6e0422c Compare February 24, 2020 10:53

sijie mentioned this pull request Feb 24, 2020

ISSUE-6409: Pull Request checks detected documentation changes but it's not streamnative/pulsar-archived#742

Closed

KannarFr force-pushed the offload-policies-per-ns branch 2 times, most recently from bed776e to 7c07d43 Compare February 24, 2020 11:27

KannarFr requested a review from codelipenghui February 24, 2020 11:28

KannarFr force-pushed the offload-policies-per-ns branch from 7c07d43 to 5447572 Compare February 24, 2020 12:08

KannarFr changed the title ~~[Issue 6283][Offloaders] WIP - Offload policies per ns with deletionLag & threshold~~ [Issue 6283][Offloaders] Offload policies per ns with deletionLag & threshold Feb 24, 2020

KannarFr force-pushed the offload-policies-per-ns branch 2 times, most recently from e6ebb2e to ad5f812 Compare February 24, 2020 22:44

KannarFr added 7 commits February 25, 2020 15:58

add features fields

8672c91

use offload policies

f452569

add features to cmdnamespace

77c6296

add features to tests

a26ce5a

use offloadpolicies in tests

3575458

remove unused

c77c6ce

if deletion lag and/or threshold are empty, use broker configuration

93769e2

KannarFr force-pushed the offload-policies-per-ns branch from 3a93d9c to c678557 Compare February 25, 2020 14:58

KannarFr added 3 commits February 25, 2020 17:40

fix tests

39f03af

manage policies

45075f1

drop tests

0114cfa

KannarFr force-pushed the offload-policies-per-ns branch from c678557 to 0114cfa Compare February 25, 2020 16:43

KannarFr closed this Feb 26, 2020

[Issue 6283][Offloaders] Offload policies per ns with deletionLag & threshold #6381

[Issue 6283][Offloaders] Offload policies per ns with deletionLag & threshold #6381

Uh oh!

Conversation

KannarFr commented Feb 20, 2020

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

KannarFr commented Feb 20, 2020

Uh oh!

codelipenghui commented Feb 22, 2020

Uh oh!

codelipenghui left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gaoran10 Feb 22, 2020

Choose a reason for hiding this comment

Uh oh!

KannarFr Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

gaoran10 Feb 22, 2020

Choose a reason for hiding this comment

Uh oh!

KannarFr Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

KannarFr commented Feb 24, 2020

Uh oh!

sijie commented Feb 24, 2020

Uh oh!

KannarFr commented Feb 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gaoran10 commented Feb 22, 2020 •

edited

Loading