You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "shibd (via GitHub)" <gi...@apache.org> on 2023/11/17 12:08:15 UTC
[PR] [fix][broker] Duplicate LedgerOffloader creation when namespace/topic… [pulsar]
shibd opened a new pull request, #21591:
URL: https://github.com/apache/pulsar/pull/21591
### Motivation
#21590
When namespace/topic policies are updated, will call this method to try to apply new policies of offload.
https://github.com/apache/pulsar/blob/eebd821e00aaabb15e0ddfa9516f854c9bfacdec/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java#L1373-L1392
But, in this case(#21590), Never hit 1380 lines. `offloader.getOffloadPolicies()` and `offloadPolicies` are always unequal.
**Reason:**
1. In #21590 reproduce step, set a useless config: `fileSystemProfilePath`, because it has been set to `managedLedgerOffloadDriver=aws-s3`.
2. This configuration is then ignored when [converted to properties](https://github.com/apache/pulsar/blob/v3.1.1/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/OffloadPoliciesImpl.java#L395-L398), then `offloader.getOffloadPolicies()` result not be included `fileSystemProfilePath`
3. `offloadPolicies` is built from the broker configuration, which contains this configuration.
4. So, the two are not equal
**Note:**
In v3.1 after. This pr: #20804 introduces a `managedLedgerExtraConfigurations` variable, which is also always unequal if the user does not set the value. (`null` vs `empty map`)
So after v3.1, In this case(#21590), this issue will appear regardless of whether you have `fileSystemProfilePath` set.
### Modifications
- Changed `toProperties` method: Regardless of the user's configuration, we should all convert.
- The `toProperties` and `create(Properties properties)` methods should be complementary.
- Fix `managedLedgerExtraConfigurations` conversion issue.
### Verifying this change
### Documentation
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Duplicate LedgerOffloader creation when namespace/topic… [pulsar]
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd merged PR #21591:
URL: https://github.com/apache/pulsar/pull/21591
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Duplicate LedgerOffloader creation when namespace/topic… [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #21591:
URL: https://github.com/apache/pulsar/pull/21591#discussion_r1398355275
##########
pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/OffloadPoliciesImpl.java:
##########
@@ -248,8 +249,7 @@ public static OffloadPoliciesImpl create(String driver, String region, String bu
public static OffloadPoliciesImpl create(Properties properties) {
OffloadPoliciesImpl data = new OffloadPoliciesImpl();
- Field[] fields = OffloadPoliciesImpl.class.getDeclaredFields();
- Arrays.stream(fields).forEach(f -> {
+ for (Field f : CONFIGURATION_FIELDS) {
Review Comment:
Since we no longer use `stream`, the code `try {} catch{}` can be moved out of the loop, right?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Duplicate LedgerOffloader creation when namespace/topic… [pulsar]
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on code in PR #21591:
URL: https://github.com/apache/pulsar/pull/21591#discussion_r1398583267
##########
pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/OffloadPoliciesImpl.java:
##########
@@ -346,66 +347,21 @@ public boolean bucketValid() {
public Properties toProperties() {
Properties properties = new Properties();
- setProperty(properties, "managedLedgerOffloadedReadPriority", this.getManagedLedgerOffloadedReadPriority());
- setProperty(properties, "offloadersDirectory", this.getOffloadersDirectory());
- setProperty(properties, "managedLedgerOffloadDriver", this.getManagedLedgerOffloadDriver());
- setProperty(properties, "managedLedgerOffloadMaxThreads",
- this.getManagedLedgerOffloadMaxThreads());
- setProperty(properties, "managedLedgerOffloadPrefetchRounds",
- this.getManagedLedgerOffloadPrefetchRounds());
- setProperty(properties, "managedLedgerOffloadThresholdInBytes",
- this.getManagedLedgerOffloadThresholdInBytes());
- setProperty(properties, "managedLedgerOffloadThresholdInSeconds",
- this.getManagedLedgerOffloadThresholdInSeconds());
- setProperty(properties, "managedLedgerOffloadDeletionLagInMillis",
- this.getManagedLedgerOffloadDeletionLagInMillis());
- setProperty(properties, "managedLedgerOffloadExtraConfigurations",
- this.getManagedLedgerExtraConfigurations());
-
- if (this.isS3Driver()) {
- setProperty(properties, "s3ManagedLedgerOffloadRegion",
- this.getS3ManagedLedgerOffloadRegion());
- setProperty(properties, "s3ManagedLedgerOffloadBucket",
- this.getS3ManagedLedgerOffloadBucket());
- setProperty(properties, "s3ManagedLedgerOffloadServiceEndpoint",
- this.getS3ManagedLedgerOffloadServiceEndpoint());
- setProperty(properties, "s3ManagedLedgerOffloadMaxBlockSizeInBytes",
- this.getS3ManagedLedgerOffloadMaxBlockSizeInBytes());
- setProperty(properties, "s3ManagedLedgerOffloadCredentialId",
- this.getS3ManagedLedgerOffloadCredentialId());
- setProperty(properties, "s3ManagedLedgerOffloadCredentialSecret",
- this.getS3ManagedLedgerOffloadCredentialSecret());
- setProperty(properties, "s3ManagedLedgerOffloadRole",
- this.getS3ManagedLedgerOffloadRole());
- setProperty(properties, "s3ManagedLedgerOffloadRoleSessionName",
- this.getS3ManagedLedgerOffloadRoleSessionName());
- setProperty(properties, "s3ManagedLedgerOffloadReadBufferSizeInBytes",
- this.getS3ManagedLedgerOffloadReadBufferSizeInBytes());
- } else if (this.isGcsDriver()) {
- setProperty(properties, "gcsManagedLedgerOffloadRegion",
- this.getGcsManagedLedgerOffloadRegion());
- setProperty(properties, "gcsManagedLedgerOffloadBucket",
- this.getGcsManagedLedgerOffloadBucket());
- setProperty(properties, "gcsManagedLedgerOffloadMaxBlockSizeInBytes",
- this.getGcsManagedLedgerOffloadMaxBlockSizeInBytes());
- setProperty(properties, "gcsManagedLedgerOffloadReadBufferSizeInBytes",
- this.getGcsManagedLedgerOffloadReadBufferSizeInBytes());
- setProperty(properties, "gcsManagedLedgerOffloadServiceAccountKeyFile",
- this.getGcsManagedLedgerOffloadServiceAccountKeyFile());
- } else if (this.isFileSystemDriver()) {
- setProperty(properties, "fileSystemProfilePath", this.getFileSystemProfilePath());
- setProperty(properties, "fileSystemURI", this.getFileSystemURI());
- }
-
- setProperty(properties, "managedLedgerOffloadBucket", this.getManagedLedgerOffloadBucket());
- setProperty(properties, "managedLedgerOffloadRegion", this.getManagedLedgerOffloadRegion());
- setProperty(properties, "managedLedgerOffloadServiceEndpoint",
- this.getManagedLedgerOffloadServiceEndpoint());
- setProperty(properties, "managedLedgerOffloadMaxBlockSizeInBytes",
- this.getManagedLedgerOffloadMaxBlockSizeInBytes());
- setProperty(properties, "managedLedgerOffloadReadBufferSizeInBytes",
- this.getManagedLedgerOffloadReadBufferSizeInBytes());
-
+ for (Field f : CONFIGURATION_FIELDS) {
+ try {
+ f.setAccessible(true);
+ if ("managedLedgerExtraConfigurations".equals(f.getName())) {
+ Map<String, String> extraConfig = (Map<String, String>) f.get(this);
+ extraConfig.forEach((key, value) -> {
+ setProperty(properties, EXTRA_CONFIG_PREFIX + key, value);
+ });
+ } else {
+ setProperty(properties, f.getName(), f.get(this));
+ }
+ } catch (Exception e) {
+ throw new IllegalArgumentException("An error occurred while processing the field: " + f.getName(), e);
+ }
+ }
Review Comment:
https://github.com/apache/pulsar/pull/21591/files#r1398582744
Here, `isS3Driver=true` but set a `fileSystemProfilePath`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Duplicate LedgerOffloader creation when namespace/topic… [pulsar]
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on code in PR #21591:
URL: https://github.com/apache/pulsar/pull/21591#discussion_r1398558813
##########
pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/OffloadPoliciesImpl.java:
##########
@@ -346,66 +347,21 @@ public boolean bucketValid() {
public Properties toProperties() {
Properties properties = new Properties();
- setProperty(properties, "managedLedgerOffloadedReadPriority", this.getManagedLedgerOffloadedReadPriority());
- setProperty(properties, "offloadersDirectory", this.getOffloadersDirectory());
- setProperty(properties, "managedLedgerOffloadDriver", this.getManagedLedgerOffloadDriver());
- setProperty(properties, "managedLedgerOffloadMaxThreads",
- this.getManagedLedgerOffloadMaxThreads());
- setProperty(properties, "managedLedgerOffloadPrefetchRounds",
- this.getManagedLedgerOffloadPrefetchRounds());
- setProperty(properties, "managedLedgerOffloadThresholdInBytes",
- this.getManagedLedgerOffloadThresholdInBytes());
- setProperty(properties, "managedLedgerOffloadThresholdInSeconds",
- this.getManagedLedgerOffloadThresholdInSeconds());
- setProperty(properties, "managedLedgerOffloadDeletionLagInMillis",
- this.getManagedLedgerOffloadDeletionLagInMillis());
- setProperty(properties, "managedLedgerOffloadExtraConfigurations",
- this.getManagedLedgerExtraConfigurations());
-
- if (this.isS3Driver()) {
- setProperty(properties, "s3ManagedLedgerOffloadRegion",
- this.getS3ManagedLedgerOffloadRegion());
- setProperty(properties, "s3ManagedLedgerOffloadBucket",
- this.getS3ManagedLedgerOffloadBucket());
- setProperty(properties, "s3ManagedLedgerOffloadServiceEndpoint",
- this.getS3ManagedLedgerOffloadServiceEndpoint());
- setProperty(properties, "s3ManagedLedgerOffloadMaxBlockSizeInBytes",
- this.getS3ManagedLedgerOffloadMaxBlockSizeInBytes());
- setProperty(properties, "s3ManagedLedgerOffloadCredentialId",
- this.getS3ManagedLedgerOffloadCredentialId());
- setProperty(properties, "s3ManagedLedgerOffloadCredentialSecret",
- this.getS3ManagedLedgerOffloadCredentialSecret());
- setProperty(properties, "s3ManagedLedgerOffloadRole",
- this.getS3ManagedLedgerOffloadRole());
- setProperty(properties, "s3ManagedLedgerOffloadRoleSessionName",
- this.getS3ManagedLedgerOffloadRoleSessionName());
- setProperty(properties, "s3ManagedLedgerOffloadReadBufferSizeInBytes",
- this.getS3ManagedLedgerOffloadReadBufferSizeInBytes());
- } else if (this.isGcsDriver()) {
- setProperty(properties, "gcsManagedLedgerOffloadRegion",
- this.getGcsManagedLedgerOffloadRegion());
- setProperty(properties, "gcsManagedLedgerOffloadBucket",
- this.getGcsManagedLedgerOffloadBucket());
- setProperty(properties, "gcsManagedLedgerOffloadMaxBlockSizeInBytes",
- this.getGcsManagedLedgerOffloadMaxBlockSizeInBytes());
- setProperty(properties, "gcsManagedLedgerOffloadReadBufferSizeInBytes",
- this.getGcsManagedLedgerOffloadReadBufferSizeInBytes());
- setProperty(properties, "gcsManagedLedgerOffloadServiceAccountKeyFile",
- this.getGcsManagedLedgerOffloadServiceAccountKeyFile());
- } else if (this.isFileSystemDriver()) {
- setProperty(properties, "fileSystemProfilePath", this.getFileSystemProfilePath());
- setProperty(properties, "fileSystemURI", this.getFileSystemURI());
- }
-
- setProperty(properties, "managedLedgerOffloadBucket", this.getManagedLedgerOffloadBucket());
- setProperty(properties, "managedLedgerOffloadRegion", this.getManagedLedgerOffloadRegion());
- setProperty(properties, "managedLedgerOffloadServiceEndpoint",
- this.getManagedLedgerOffloadServiceEndpoint());
- setProperty(properties, "managedLedgerOffloadMaxBlockSizeInBytes",
- this.getManagedLedgerOffloadMaxBlockSizeInBytes());
- setProperty(properties, "managedLedgerOffloadReadBufferSizeInBytes",
- this.getManagedLedgerOffloadReadBufferSizeInBytes());
-
+ for (Field f : CONFIGURATION_FIELDS) {
+ try {
+ f.setAccessible(true);
+ if ("managedLedgerExtraConfigurations".equals(f.getName())) {
+ Map<String, String> extraConfig = (Map<String, String>) f.get(this);
+ extraConfig.forEach((key, value) -> {
+ setProperty(properties, EXTRA_CONFIG_PREFIX + key, value);
+ });
+ } else {
+ setProperty(properties, f.getName(), f.get(this));
+ }
+ } catch (Exception e) {
+ throw new IllegalArgumentException("An error occurred while processing the field: " + f.getName(), e);
+ }
+ }
Review Comment:
Do we have the tests to cover the left case?
such as `isS3Driver=true`, but `gcsManagedLedgerOffloadRegion=west`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Duplicate LedgerOffloader creation when namespace/topic… [pulsar]
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on code in PR #21591:
URL: https://github.com/apache/pulsar/pull/21591#discussion_r1398582744
##########
pulsar-common/src/test/java/org/apache/pulsar/common/policies/data/OffloadPoliciesTest.java:
##########
@@ -436,13 +439,37 @@ private byte[] loadClassData(String name) throws IOException {
@Test
public void testCreateOffloadPoliciesWithExtraConfiguration() {
Properties properties = new Properties();
- properties.put("managedLedgerOffloadExtraConfigKey1", "value1");
- properties.put("managedLedgerOffloadExtraConfigKey2", "value2");
+ properties.put(EXTRA_CONFIG_PREFIX + "Key1", "value1");
+ properties.put(EXTRA_CONFIG_PREFIX + "Key2", "value2");
OffloadPoliciesImpl policies = OffloadPoliciesImpl.create(properties);
Map<String, String> extraConfigurations = policies.getManagedLedgerExtraConfigurations();
Assert.assertEquals(extraConfigurations.size(), 2);
Assert.assertEquals(extraConfigurations.get("Key1"), "value1");
Assert.assertEquals(extraConfigurations.get("Key2"), "value2");
}
+
+ /**
+ * Test toProperties as well as create from properties.
+ * @throws Exception
+ */
+ @Test
+ public void testToProperties() throws Exception {
+ // Base information convert.
+ OffloadPoliciesImpl offloadPolicies = OffloadPoliciesImpl.create("aws-s3", "test-region", "test-bucket",
+ "http://test.endpoint", null, null, null, null, 32 * 1024 * 1024, 5 * 1024 * 1024,
+ 10 * 1024 * 1024L, 100L, 10000L, OffloadedReadPriority.TIERED_STORAGE_FIRST);
+ assertEquals(offloadPolicies, OffloadPoliciesImpl.create(offloadPolicies.toProperties()));
+
+ // Set useless config to offload policies. Make sure convert conversion result is the same.
Review Comment:
This example covers the scenario when the configuration does not match.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Duplicate LedgerOffloader creation when namespace/topic… [pulsar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #21591:
URL: https://github.com/apache/pulsar/pull/21591#issuecomment-1818147723
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/21591?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#21591](https://app.codecov.io/gh/apache/pulsar/pull/21591?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (652999e) into [master](https://app.codecov.io/gh/apache/pulsar/commit/403faa4c77862062e638e95fc36c4ee53c5b8f41?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (403faa4) will **decrease** coverage by `0.09%`.
> Report is 1 commits behind head on master.
> The diff coverage is `90.47%`.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/pulsar/pull/21591?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #21591 +/- ##
============================================
- Coverage 73.35% 73.27% -0.09%
+ Complexity 32711 32685 -26
============================================
Files 1892 1892
Lines 140707 140663 -44
Branches 15483 15484 +1
============================================
- Hits 103211 103066 -145
- Misses 29394 29493 +99
- Partials 8102 8104 +2
```
| [Flag](https://app.codecov.io/gh/apache/pulsar/pull/21591/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [inttests](https://app.codecov.io/gh/apache/pulsar/pull/21591/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.19% <28.57%> (-0.06%)` | :arrow_down: |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/21591/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.68% <71.42%> (-0.38%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/21591/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.58% <90.47%> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files](https://app.codecov.io/gh/apache/pulsar/pull/21591?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...lsar/common/policies/data/OffloadPoliciesImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/21591?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NvbW1vbi9wb2xpY2llcy9kYXRhL09mZmxvYWRQb2xpY2llc0ltcGwuamF2YQ==) | `85.66% <90.47%> (+2.02%)` | :arrow_up: |
... and [78 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/21591/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Duplicate LedgerOffloader creation when namespace/topic… [pulsar]
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on code in PR #21591:
URL: https://github.com/apache/pulsar/pull/21591#discussion_r1398534161
##########
pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/OffloadPoliciesImpl.java:
##########
@@ -248,8 +249,7 @@ public static OffloadPoliciesImpl create(String driver, String region, String bu
public static OffloadPoliciesImpl create(Properties properties) {
OffloadPoliciesImpl data = new OffloadPoliciesImpl();
- Field[] fields = OffloadPoliciesImpl.class.getDeclaredFields();
- Arrays.stream(fields).forEach(f -> {
+ for (Field f : CONFIGURATION_FIELDS) {
Review Comment:
The main thing here is to convert the exception to RuntimeException.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Duplicate LedgerOffloader creation when namespace/topic… [pulsar]
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd closed pull request #21591: [fix][broker] Duplicate LedgerOffloader creation when namespace/topic…
URL: https://github.com/apache/pulsar/pull/21591
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org