Pulsar: managedLedgerOffloadDeletionLagMs and managedLedgerOffloadAutoTriggerSizeThresholdBytes are not used from config file

[milos-matijasevic]

Describe the bug
Flags managedLedgerOffloadDeletionLagMs and managedLedgerOffloadAutoTriggerSizeThresholdBytes are not used anywhere so default offloader doesn't pick these values properly and always use defaults null and -1 respectively, even if these flags are set in config file.

To Reproduce
Deploy pulsar and set these values in broker.conf file (https://github.com/apache/pulsar/blob/master/conf/broker.conf#L812) to some values different from default, also set offloader settings (https://github.com/apache/pulsar/blob/master/conf/broker.conf#L1070) so it can be tested.
Try to produce data and check if ledgers are being offloaded if u set managedLedgerOffloadAutoTriggerSizeThresholdBytes to 0.

Expected behavior
Offloader should use values provided from config file.
In my test case if i set managedLedgerOffloadAutoTriggerSizeThresholdBytes to 0, offloads should be done immediately after ledger rollover which i inspect with stats-internal and ledgers are properly closed. But that don't happen unless i manually set offload threshold with set-offload-threshold command from pulsar admin, after that everything work as expected.

Screenshots
For above test case, before and after i change offload manually states are: (first ledger is offloaded manually)

Desktop (please complete the following information):

• OS: Linux

Additional context
These 2 flags are not read anywhere, in previous implementation they are taken here https://github.com/apache/pulsar/blob/ffe29c30d8963850770f55db86bbcd9ea2f2a095/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L1009 but this part is now removed, i think that should be inserted here https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L1214.

Also these 2 flags are, in my opinion, more closely part of offloading config instead of managed ledger config, so other solution can be that these 2 parameters are moved in offloader part of configuration and renamed so this function https://github.com/apache/pulsar/blob/ffe29c30d8963850770f55db86bbcd9ea2f2a095/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/OffloadPolicies.java#L101 can pick them up properly.

One more thing, values for these 2 flags are stored both in OffloadPolicies class and Policies class (https://github.com/apache/pulsar/blob/master/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/Policies.java#L95), and in Policies class they are not used for anything except for reading that value for pulsar admin get command (https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java#L2672). And they are set (https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java#L2699) at the same time so maybe they are redundant and maybe better solution is to move them to offload config and remove them from Policies class.

[milos-matijasevic]

@codelipenghui hello, i saw that you removed this piece of code and wrote this explanation in pull request https://github.com/apache/pulsar/pull/7011#discussion_r428788947, can you explain me how things will work without this and how will values for these 2 parameters be taken from config file, you can read my issue and see why i think it won't work

[cdbartholomew]

@milos-matijasevic Can you try the following names for these properties in your broker.conf file:

managedLedgerOffloadThresholdInBytes
managedLedgerOffloadDeletionLagInMillis

I think the names have been changed.

[milos-matijasevic]

Hello, actually i tried that names but it didn't work, as i mentioned in issue if they are renamed like that i think it should work, but they are not. I see old name in config file (https://github.com/apache/pulsar/blob/master/conf/broker.conf#L812) and in code (https://github.com/apache/pulsar/blob/master/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java#L1291)
If i understand, these 2 files are relevant for loading configuration?

They are renamed here like you said (https://github.com/apache/pulsar/blob/master/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/OffloadPolicies.java#L54), but in order for them to be loaded from config file, they should be renamed in 2 files that i mentioned above too.

[gaoran10]

@milos-matijasevic Thanks for your work. I think you are right, the configuration names in the ServiceConfiguration are not matched with field names in the OffloadPolicies, so the configurations in the OffloadPolicies can't be set properly through java reflection refer to.

# ServiceConfiguration (broker.conf / standalone.conf)
managedLedgerOffloadDeletionLagMs
managedLedgerOffloadAutoTriggerSizeThresholdBytes

# OffloadPolicies
managedLedgerOffloadDeletionLagInMillis
managedLedgerOffloadThresholdInBytes

[addisonj]

@gaoran10 We should make sure we support the original value names (managedLedgerOffloadAutoTriggerSizeThresholdBytes) as well as the new value names (managedLedgerOffloadThresholdInBytes) from the broker.conf, I think we should fix this by either:

A) just going back to use the constructor here: https://github.com/apache/pulsar/blob/9f687d3f67d7534cb9ee77ddd2c7d0f24d34d0b4/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/OffloadPolicies.java#L79 and override the getter for getManagedLedgerOffloadAutoTriggerSizeThresholdBytes() to also look at the other properties OR

B) add a method to ServiceConfiguration that builds up all the relevant properties. Instead of using the .getProperties() method we could build .getOffloadPolicyProperties() which would normalize the values.

Personally, I think I prefer B.

Also, just an an FYI, I talked with @milos-matijasevic and one improvement we could make is that when we construct a new namespace policy with the pulsar-admin namespaces set-offload-policies that you wouldn't need to set all the properties and instead if you don't specify an argument, it will just merge it from the cluster default policy, but we should consider that a separate improvement

[gaoran10]

@addisonj Thanks, I prefer the approach B too. The configuration names in config files (broker.conf or standalone.conf) are original names, we only make sure that the fields' value in the OffloadPolicies could be set properly.

Yes, about the configurations cover problem, we could fix it in another PR.

[milos-matijasevic]

Thank you guys,
As @addisonj mentioned we spoke (thanks a lot for effort) about improvements and i have some things that may be good

• command for unsetting offload policies for namespace so it can go back to default pulsar-admin namespaces unset-offload-policies
• command for getting default offload policies(EDIT: this actually you can see in broker.conf so it is not that important)
• Sparse properties saved for namespaces, if property is set for namespace use it, if not go back to default. (this one maybe is not that important but it is nice for this usecase: i wan't for every namespace to use default driver, bucket, etc.. and only to change offload threshold, so if i use set-offload-threshold, only threshold can be saved in namespace offload properties, and then if i want to change bucket or other common property for namespaces i can only change it in default (from configuration) and won't need to do it for every namespace like i need now)

[gaoran10]

@milos-matijasevic @addisonj I added an issue to record the offload configuration setting improment, PTAL. https://github.com/streamnative/pulsar/issues/1578

Maybe we could fix this issue first and make the improvements in other PRs.