You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "thetumbled (via GitHub)" <gi...@apache.org> on 2023/08/28 08:47:14 UTC
[GitHub] [pulsar] thetumbled opened a new pull request, #21078: [fix] [broker] remove bundle-data in local metadata store.
thetumbled opened a new pull request, #21078:
URL: https://github.com/apache/pulsar/pull/21078
### Motivation
When deleting a namespace, we will delete znode under the path /loadbalance/bundle-data in `local metadata store` instead of `global`,.
### Modifications
Delete bundle data znode in local metadata store.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change is a trivial rework / code cleanup without any test coverage.
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: <!-- ENTER URL HERE -->
--
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
[GitHub] [pulsar] thetumbled commented on a diff in pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#discussion_r1311023028
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/NamespaceResources.java:
##########
@@ -381,13 +383,13 @@ public CompletableFuture<Void> runWithMarkDeleteAsync(TopicName topic,
// clear resource of `/loadbalance/bundle-data/{tenant}/{namespace}/` in metadata-store
public CompletableFuture<Void> deleteBundleDataAsync(NamespaceName ns) {
final String namespaceBundlePath = joinPath(BUNDLE_DATA_BASE_PATH, ns.toString());
- return getStore().deleteRecursive(namespaceBundlePath);
Review Comment:
Not wrong path, but wrong places. There are two metadata store: local store and global zk.
When deleting a namespace, we will delete znode under the path `/loadbalance/bundle-data` in `local` metadata store instead of `global` metadata store.
--
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
[GitHub] [pulsar] thetumbled commented on pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#issuecomment-1695300384
PTAL, thanks. @codelipenghui @hangc0276 @AnonHxy
--
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] remove bundle-data in local metadata store. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#issuecomment-1773983056
> @thetumbled Can you help create a PR to cherry-pick this change to `branch-2.11`?
Sure.
--
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
[GitHub] [pulsar] poorbarcode commented on pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#issuecomment-1697029692
@heesung-sn @Demogorgon314 Could you take a look? Thanks
--
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
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#discussion_r1309875454
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApi2Test.java:
##########
@@ -1740,6 +1750,20 @@ public void testDeleteNamespace(NamespaceAttr namespaceAttr) throws Exception {
admin.topics().createPartitionedTopic(topic, 10);
assertFalse(admin.topics().getList(namespace).isEmpty());
+ // export bundle-data to metadata store
+ final String managedLedgersPath = "/managed-ledgers/" + namespace;
+ final String bundleDataPath = "/loadbalance/bundle-data/" + namespace;
+ ModularLoadManagerImpl loadManager = (ModularLoadManagerImpl) getField(
+ pulsar.getLoadManager().get(), "loadManager");
+ MetadataCache<BundleData> bundlesCache = (MetadataCache<BundleData>) getField(
+ loadManager, "bundlesCache");
+ final BundleData bundleData = new BundleData(10, 1000);
+ bundlesCache.readModifyUpdateOrCreate(bundleDataPath, __ -> bundleData).join();
Review Comment:
```suggestion
// Trigger bundle owned by brokers.
pulsarClient.newProducer().topic(topic).create().close();
// Trigger bundle data write to ZK.
Awaitility.await().untilAsserted(() -> {
boolean bundleDataWereWriten = false;
for (PulsarService ps : new PulsarService[]{pulsar, mockPulsarSetup.getPulsar()}) {
ModularLoadManagerWrapper loadManager = (ModularLoadManagerWrapper) ps.getLoadManager().get();
ModularLoadManagerImpl loadManagerImpl = (ModularLoadManagerImpl) loadManager.getLoadManager();
ps.getBrokerService().updateRates();
loadManagerImpl.updateLocalBrokerData();
loadManagerImpl.writeBundleDataOnZooKeeper();
bundleDataWereWriten = bundleDataWereWriten || ps.getLocalMetadataStore().exists(bundleDataPath).join();
}
assertTrue(bundleDataWereWriten);
});
```
--
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
[GitHub] [pulsar] AnonHxy commented on a diff in pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "AnonHxy (via GitHub)" <gi...@apache.org>.
AnonHxy commented on code in PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#discussion_r1309956066
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/NamespaceResources.java:
##########
@@ -49,16 +49,18 @@ public class NamespaceResources extends BaseResources<Policies> {
private final IsolationPolicyResources isolationPolicies;
private final PartitionedTopicResources partitionedTopicResources;
private final MetadataStore configurationStore;
+ private final MetadataStore localStore;
public static final String POLICIES_READONLY_FLAG_PATH = "/admin/flags/policies-readonly";
private static final String NAMESPACE_BASE_PATH = "/namespace";
private static final String BUNDLE_DATA_BASE_PATH = "/loadbalance/bundle-data";
- public NamespaceResources(MetadataStore configurationStore, int operationTimeoutSec) {
+ public NamespaceResources(MetadataStore localStore, MetadataStore configurationStore, int operationTimeoutSec) {
Review Comment:
OK. As a bug fix, it change could fix the problem and LGTM. Maybe we could refactor in another PR
--
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
[GitHub] [pulsar] thetumbled commented on a diff in pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#discussion_r1309984217
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApi2Test.java:
##########
@@ -1713,13 +1714,21 @@ private void setNamespaceAttr(NamespaceAttr namespaceAttr){
conf.setForceDeleteNamespaceAllowed(namespaceAttr.forceDeleteNamespaceAllowed);
}
+ private static Object getField(Object obj, String fieldName) throws Exception {
+ Field field = obj.getClass().getDeclaredField(fieldName);
+ field.setAccessible(true);
+ return field.get(obj);
+ }
+
Review Comment:
fixed.
--
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
[GitHub] [pulsar] thetumbled commented on pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#issuecomment-1704520782
> I need to confirm something related to this. The namespace-level metadata should store all the data in the configuration store. You change deleting from the configuration store to the local store. How can we delete it in the configuration store?
There is no such bundle-data in the configuration store. Znodes for bundle-data is created by `LoadManager` in local store, instead of configuration store.
--
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
[GitHub] [pulsar] AnonHxy commented on a diff in pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "AnonHxy (via GitHub)" <gi...@apache.org>.
AnonHxy commented on code in PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#discussion_r1309956066
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/NamespaceResources.java:
##########
@@ -49,16 +49,18 @@ public class NamespaceResources extends BaseResources<Policies> {
private final IsolationPolicyResources isolationPolicies;
private final PartitionedTopicResources partitionedTopicResources;
private final MetadataStore configurationStore;
+ private final MetadataStore localStore;
public static final String POLICIES_READONLY_FLAG_PATH = "/admin/flags/policies-readonly";
private static final String NAMESPACE_BASE_PATH = "/namespace";
private static final String BUNDLE_DATA_BASE_PATH = "/loadbalance/bundle-data";
- public NamespaceResources(MetadataStore configurationStore, int operationTimeoutSec) {
+ public NamespaceResources(MetadataStore localStore, MetadataStore configurationStore, int operationTimeoutSec) {
Review Comment:
OK. As a bug fix, this change could fix the problem and it LGTM. Maybe we could refactor it in another PR
--
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
[GitHub] [pulsar] mattisonchao commented on pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#issuecomment-1704524226
Btw, I suggest we move this logic out of the namespace resource. Because we didn't create bundle data in NamespaceResource but deleted it there, it's weird.
--
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
[GitHub] [pulsar] thetumbled commented on a diff in pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#discussion_r1308356451
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/NamespaceResources.java:
##########
@@ -49,16 +49,18 @@ public class NamespaceResources extends BaseResources<Policies> {
private final IsolationPolicyResources isolationPolicies;
private final PartitionedTopicResources partitionedTopicResources;
private final MetadataStore configurationStore;
+ private final MetadataStore localStore;
public static final String POLICIES_READONLY_FLAG_PATH = "/admin/flags/policies-readonly";
private static final String NAMESPACE_BASE_PATH = "/namespace";
private static final String BUNDLE_DATA_BASE_PATH = "/loadbalance/bundle-data";
- public NamespaceResources(MetadataStore configurationStore, int operationTimeoutSec) {
+ public NamespaceResources(MetadataStore localStore, MetadataStore configurationStore, int operationTimeoutSec) {
Review Comment:
1. It seems that we can't allow the `configurationMetadataStore` to be null, or there will be serious issues.
https://github.com/apache/pulsar/blob/d099ac4fa2f217b9c5f0a5e660c83048e829c5d7/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java#L752-L762
2. Currently, bundle-data are created/updated by `LoadManager`, and deleted by `LoadManager` and `NamespaceResources`. It is beneficial to introduce a `BundleResources` to unify the opeartions to bundle-data. But i think that it should be introduced as a code optimization instead of bug fix, which need to be discussed fully.
--
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
[GitHub] [pulsar] thetumbled commented on a diff in pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#discussion_r1309892997
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApi2Test.java:
##########
@@ -1740,6 +1750,20 @@ public void testDeleteNamespace(NamespaceAttr namespaceAttr) throws Exception {
admin.topics().createPartitionedTopic(topic, 10);
assertFalse(admin.topics().getList(namespace).isEmpty());
+ // export bundle-data to metadata store
+ final String managedLedgersPath = "/managed-ledgers/" + namespace;
+ final String bundleDataPath = "/loadbalance/bundle-data/" + namespace;
+ ModularLoadManagerImpl loadManager = (ModularLoadManagerImpl) getField(
+ pulsar.getLoadManager().get(), "loadManager");
+ MetadataCache<BundleData> bundlesCache = (MetadataCache<BundleData>) getField(
+ loadManager, "bundlesCache");
+ final BundleData bundleData = new BundleData(10, 1000);
+ bundlesCache.readModifyUpdateOrCreate(bundleDataPath, __ -> bundleData).join();
Review Comment:
done.
--
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
[GitHub] [pulsar] thetumbled commented on a diff in pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#discussion_r1308356451
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/NamespaceResources.java:
##########
@@ -49,16 +49,18 @@ public class NamespaceResources extends BaseResources<Policies> {
private final IsolationPolicyResources isolationPolicies;
private final PartitionedTopicResources partitionedTopicResources;
private final MetadataStore configurationStore;
+ private final MetadataStore localStore;
public static final String POLICIES_READONLY_FLAG_PATH = "/admin/flags/policies-readonly";
private static final String NAMESPACE_BASE_PATH = "/namespace";
private static final String BUNDLE_DATA_BASE_PATH = "/loadbalance/bundle-data";
- public NamespaceResources(MetadataStore configurationStore, int operationTimeoutSec) {
+ public NamespaceResources(MetadataStore localStore, MetadataStore configurationStore, int operationTimeoutSec) {
Review Comment:
1. It seems that we can't allow the `configurationMetadataStore` to be null, or there will be serious issues.
https://github.com/apache/pulsar/blob/d099ac4fa2f217b9c5f0a5e660c83048e829c5d7/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java#L752-L762
2. Currently, bundle-data are created/updated by `LoadManager`, and deleted by `LoadManager` and `NamespaceResources`. **It is beneficial to introduce a `BundleResources` to unify the opeartions to bundle-data. But i think that it should be introduced as a code optimization instead of bug fix, which need to be discussed fully.**
--
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
[GitHub] [pulsar] codecov-commenter commented on pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#issuecomment-1699415516
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/21078?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#21078](https://app.codecov.io/gh/apache/pulsar/pull/21078?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (cc9bf78) into [master](https://app.codecov.io/gh/apache/pulsar/commit/d06cda6cd8a58b8a7e0678183f05c08059ddb9b2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (d06cda6) will **increase** coverage by `38.42%`.
> Report is 30 commits behind head on master.
> The diff coverage is `65.17%`.
[](https://app.codecov.io/gh/apache/pulsar/pull/21078?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #21078 +/- ##
=============================================
+ Coverage 34.70% 73.13% +38.42%
- Complexity 12121 32370 +20249
=============================================
Files 1698 1887 +189
Lines 129914 139862 +9948
Branches 14172 15384 +1212
=============================================
+ Hits 45089 102289 +57200
+ Misses 78825 29475 -49350
- Partials 6000 8098 +2098
```
| [Flag](https://app.codecov.io/gh/apache/pulsar/pull/21078/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/21078/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.06% <7.46%> (?)` | |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/21078/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `25.15% <5.97%> (+0.04%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/21078/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.41% <65.17%> (+40.32%)` | :arrow_up: |
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 Changed](https://app.codecov.io/gh/apache/pulsar/pull/21078?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/21078?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9pbXBsL01hbmFnZWRMZWRnZXJJbXBsLmphdmE=) | `81.21% <0.00%> (+34.26%)` | :arrow_up: |
| [...ookkeeper/PulsarLedgerUnderreplicationManager.java](https://app.codecov.io/gh/apache/pulsar/pull/21078?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLW1ldGFkYXRhL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvbWV0YWRhdGEvYm9va2tlZXBlci9QdWxzYXJMZWRnZXJVbmRlcnJlcGxpY2F0aW9uTWFuYWdlci5qYXZh) | `51.74% <52.85%> (+51.74%)` | :arrow_up: |
| [...etadata/bookkeeper/PulsarLedgerManagerFactory.java](https://app.codecov.io/gh/apache/pulsar/pull/21078?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLW1ldGFkYXRhL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvbWV0YWRhdGEvYm9va2tlZXBlci9QdWxzYXJMZWRnZXJNYW5hZ2VyRmFjdG9yeS5qYXZh) | `61.53% <57.14%> (+30.98%)` | :arrow_up: |
| [...ookkeeper/LongHierarchicalLedgerRangeIterator.java](https://app.codecov.io/gh/apache/pulsar/pull/21078?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLW1ldGFkYXRhL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvbWV0YWRhdGEvYm9va2tlZXBlci9Mb25nSGllcmFyY2hpY2FsTGVkZ2VyUmFuZ2VJdGVyYXRvci5qYXZh) | `81.66% <66.66%> (+81.66%)` | :arrow_up: |
| [...metadata/bookkeeper/PulsarRegistrationManager.java](https://app.codecov.io/gh/apache/pulsar/pull/21078?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLW1ldGFkYXRhL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvbWV0YWRhdGEvYm9va2tlZXBlci9QdWxzYXJSZWdpc3RyYXRpb25NYW5hZ2VyLmphdmE=) | `62.56% <70.96%> (+53.16%)` | :arrow_up: |
| [...kkeeper/LegacyHierarchicalLedgerRangeIterator.java](https://app.codecov.io/gh/apache/pulsar/pull/21078?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLW1ldGFkYXRhL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvbWV0YWRhdGEvYm9va2tlZXBlci9MZWdhY3lIaWVyYXJjaGljYWxMZWRnZXJSYW5nZUl0ZXJhdG9yLmphdmE=) | `72.15% <71.42%> (+72.15%)` | :arrow_up: |
| [...he/pulsar/broker/resources/NamespaceResources.java](https://app.codecov.io/gh/apache/pulsar/pull/21078?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3B1bHNhci9icm9rZXIvcmVzb3VyY2VzL05hbWVzcGFjZVJlc291cmNlcy5qYXZh) | `80.28% <75.00%> (+10.28%)` | :arrow_up: |
| [...ulsar/metadata/bookkeeper/PulsarLayoutManager.java](https://app.codecov.io/gh/apache/pulsar/pull/21078?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLW1ldGFkYXRhL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvbWV0YWRhdGEvYm9va2tlZXBlci9QdWxzYXJMYXlvdXRNYW5hZ2VyLmphdmE=) | `66.66% <77.77%> (+46.66%)` | :arrow_up: |
| [...ker/service/persistent/PersistentSubscription.java](https://app.codecov.io/gh/apache/pulsar/pull/21078?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3BlcnNpc3RlbnQvUGVyc2lzdGVudFN1YnNjcmlwdGlvbi5qYXZh) | `75.89% <83.33%> (+26.20%)` | :arrow_up: |
| [...pache/pulsar/broker/resources/PulsarResources.java](https://app.codecov.io/gh/apache/pulsar/pull/21078?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3B1bHNhci9icm9rZXIvcmVzb3VyY2VzL1B1bHNhclJlc291cmNlcy5qYXZh) | `83.72% <100.00%> (ø)` | |
| ... and [3 more](https://app.codecov.io/gh/apache/pulsar/pull/21078?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
... and [1459 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/21078/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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
[GitHub] [pulsar] AnonHxy commented on pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "AnonHxy (via GitHub)" <gi...@apache.org>.
AnonHxy commented on PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#issuecomment-1704885230
> Btw, I suggest we move this logic out of the namespace resource. Because we didn't create bundle data in NamespaceResource but deleted it there, it's weird.
+1. As I mentioned above https://github.com/apache/pulsar/pull/21078#discussion_r1308129685. I have create a new PR to refactor this https://github.com/apache/pulsar/pull/21119
--
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
[GitHub] [pulsar] Demogorgon314 commented on a diff in pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "Demogorgon314 (via GitHub)" <gi...@apache.org>.
Demogorgon314 commented on code in PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#discussion_r1309980016
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApi2Test.java:
##########
@@ -1713,13 +1714,21 @@ private void setNamespaceAttr(NamespaceAttr namespaceAttr){
conf.setForceDeleteNamespaceAllowed(namespaceAttr.forceDeleteNamespaceAllowed);
}
+ private static Object getField(Object obj, String fieldName) throws Exception {
+ Field field = obj.getClass().getDeclaredField(fieldName);
+ field.setAccessible(true);
+ return field.get(obj);
+ }
+
Review Comment:
Unused code.
--
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
[GitHub] [pulsar] AnonHxy commented on a diff in pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "AnonHxy (via GitHub)" <gi...@apache.org>.
AnonHxy commented on code in PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#discussion_r1308129685
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/NamespaceResources.java:
##########
@@ -49,16 +49,18 @@ public class NamespaceResources extends BaseResources<Policies> {
private final IsolationPolicyResources isolationPolicies;
private final PartitionedTopicResources partitionedTopicResources;
private final MetadataStore configurationStore;
+ private final MetadataStore localStore;
public static final String POLICIES_READONLY_FLAG_PATH = "/admin/flags/policies-readonly";
private static final String NAMESPACE_BASE_PATH = "/namespace";
private static final String BUNDLE_DATA_BASE_PATH = "/loadbalance/bundle-data";
- public NamespaceResources(MetadataStore configurationStore, int operationTimeoutSec) {
+ public NamespaceResources(MetadataStore localStore, MetadataStore configurationStore, int operationTimeoutSec) {
Review Comment:
How about creating a separate class naming `BundleResources` to handle the bundle-date creating and deleting. The reason is that:
1. `NamespaceResources` will be null if `configurationMetadataStore != null` is false, so there is a chance to throw NPE when call `NamespaceResources.deleteBundleDataAsync(..)` or `NamespaceResources.deleteBundleDataTenantAsync(...)`:
https://github.com/apache/pulsar/blob/d099ac4fa2f217b9c5f0a5e660c83048e829c5d7/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/PulsarResources.java#L60-L64
2. `NamespaceResources#BUNDLE_DATA_BASE_PATH` and `ModularLoadManagerImpl#BUNDLE_DATA_PATH` are duplicated. `BundleResources` could unify them. WDYT
--
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
[GitHub] [pulsar] thetumbled commented on a diff in pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#discussion_r1311023028
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/NamespaceResources.java:
##########
@@ -381,13 +383,13 @@ public CompletableFuture<Void> runWithMarkDeleteAsync(TopicName topic,
// clear resource of `/loadbalance/bundle-data/{tenant}/{namespace}/` in metadata-store
public CompletableFuture<Void> deleteBundleDataAsync(NamespaceName ns) {
final String namespaceBundlePath = joinPath(BUNDLE_DATA_BASE_PATH, ns.toString());
- return getStore().deleteRecursive(namespaceBundlePath);
Review Comment:
Not wrong path, but wrong places. There are two metadata store: local store and global zk.
When deleting a namespace, we should delete znode under the path `/loadbalance/bundle-data` in `local` metadata store instead of `global` metadata store.
--
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
[GitHub] [pulsar] poorbarcode commented on pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#issuecomment-1699410604
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] poorbarcode commented on pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#issuecomment-1699328995
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] poorbarcode merged pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode merged PR #21078:
URL: https://github.com/apache/pulsar/pull/21078
--
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
[GitHub] [pulsar] heesung-sn commented on a diff in pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on code in PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#discussion_r1310548082
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/NamespaceResources.java:
##########
@@ -381,13 +383,13 @@ public CompletableFuture<Void> runWithMarkDeleteAsync(TopicName topic,
// clear resource of `/loadbalance/bundle-data/{tenant}/{namespace}/` in metadata-store
public CompletableFuture<Void> deleteBundleDataAsync(NamespaceName ns) {
final String namespaceBundlePath = joinPath(BUNDLE_DATA_BASE_PATH, ns.toString());
- return getStore().deleteRecursive(namespaceBundlePath);
Review Comment:
Just to confirm this logic change,
Previously, we have not deleted any bundle data because we were deleting it in the wrong path here?
--
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
[GitHub] [pulsar] mattisonchao commented on pull request #21078: [fix] [broker] remove bundle-data in local metadata store.
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#issuecomment-1704523012
Yep, you are right. I was misunderstood.
--
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] remove bundle-data in local metadata store. [pulsar]
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on PR #21078:
URL: https://github.com/apache/pulsar/pull/21078#issuecomment-1773978425
@thetumbled Can you help create a PR to cherry-pick this change to `branch-2.11`?
--
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