You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/08/20 09:12:51 UTC
[GitHub] [pulsar] horizonzy opened a new pull request, #17192: [fix][broker] Fix pulsarLedgerIdGenerator can't delete index path when zk metadata store config rootPath.
horizonzy opened a new pull request, #17192:
URL: https://github.com/apache/pulsar/pull/17192
*(If this PR fixes a github issue, please add `Fixes #<xyz>`.)*
Fixes #17191
### Motivation
Fix pulsarLedgerIdGenerator can't delete index path when zk metadata store config rootPath.
### Documentation
- [ ] `doc-not-needed`
(Please explain why)
--
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] horizonzy commented on a diff in pull request #17192: [fix][broker] Fix pulsarLedgerIdGenerator can't delete index path when zk metadata store config rootPath.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #17192:
URL: https://github.com/apache/pulsar/pull/17192#discussion_r952224104
##########
pulsar-metadata/src/test/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerIdGeneratorTest.java:
##########
@@ -108,19 +109,118 @@ public void testGenerateLedgerId(String provider, Supplier<String> urlSupplier)
assertTrue(countDownLatch2.await(120, TimeUnit.SECONDS),
"Wait ledger id generation threads to stop timeout : ");
- log.info("Number of generated ledger id: {}, time used: {}", ledgerIds.size(),
+ log.info("Number of generated ledger id: {}, time used: {}", shortLedgerIds.size() + longLedgerIds.size(),
System.currentTimeMillis() - start);
assertEquals(errCount.get(), 0, "Error occur during ledger id generation : ");
Set<Long> ledgers = new HashSet<>();
- while (!ledgerIds.isEmpty()) {
- Long ledger = ledgerIds.poll();
+ while (!shortLedgerIds.isEmpty()) {
+ Long ledger = shortLedgerIds.poll();
+ assertNotNull(ledger, "Generated ledger id is null");
+ assertFalse(ledgers.contains(ledger), "Ledger id [" + ledger + "] conflict : ");
+ ledgers.add(ledger);
+ }
+ while (!longLedgerIds.isEmpty()) {
+ Long ledger = longLedgerIds.poll();
+ assertNotNull(ledger, "Generated ledger id is null");
+ assertFalse(ledgers.contains(ledger), "Ledger id [" + ledger + "] conflict : ");
+ ledgers.add(ledger);
+ }
+ }
+
+ @Test
+ public void testGenerateLedgerIdWithZkPrefix() throws Exception {
+ @Cleanup
+ MetadataStoreExtended store =
Review Comment:
In testGenerateLedgerIdWithZkPrefix, we need check the index path is still exists.
--
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] Jason918 commented on a diff in pull request #17192: [fix][broker] Fix pulsarLedgerIdGenerator can't delete index path when zk metadata store config rootPath.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #17192:
URL: https://github.com/apache/pulsar/pull/17192#discussion_r952219796
##########
pulsar-metadata/src/test/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerIdGeneratorTest.java:
##########
@@ -108,19 +109,118 @@ public void testGenerateLedgerId(String provider, Supplier<String> urlSupplier)
assertTrue(countDownLatch2.await(120, TimeUnit.SECONDS),
"Wait ledger id generation threads to stop timeout : ");
- log.info("Number of generated ledger id: {}, time used: {}", ledgerIds.size(),
+ log.info("Number of generated ledger id: {}, time used: {}", shortLedgerIds.size() + longLedgerIds.size(),
System.currentTimeMillis() - start);
assertEquals(errCount.get(), 0, "Error occur during ledger id generation : ");
Set<Long> ledgers = new HashSet<>();
- while (!ledgerIds.isEmpty()) {
- Long ledger = ledgerIds.poll();
+ while (!shortLedgerIds.isEmpty()) {
+ Long ledger = shortLedgerIds.poll();
+ assertNotNull(ledger, "Generated ledger id is null");
+ assertFalse(ledgers.contains(ledger), "Ledger id [" + ledger + "] conflict : ");
+ ledgers.add(ledger);
+ }
+ while (!longLedgerIds.isEmpty()) {
+ Long ledger = longLedgerIds.poll();
+ assertNotNull(ledger, "Generated ledger id is null");
+ assertFalse(ledgers.contains(ledger), "Ledger id [" + ledger + "] conflict : ");
+ ledgers.add(ledger);
+ }
+ }
+
+ @Test
+ public void testGenerateLedgerIdWithZkPrefix() throws Exception {
+ @Cleanup
+ MetadataStoreExtended store =
Review Comment:
If this line is the only difference between `testGenerateLedgerId` and `testGenerateLedgerIdWithZkPrefix`, we can reuse the common codes 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] codelipenghui commented on a diff in pull request #17192: [fix][broker] Fix pulsarLedgerIdGenerator can't delete index path when zk metadata store config rootPath.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #17192:
URL: https://github.com/apache/pulsar/pull/17192#discussion_r951144635
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerIdGenerator.java:
##########
@@ -287,4 +288,16 @@ private static String createLedgerPrefix(String ledgersPath, String idGenZnodeNa
return ledgerIdGenPath + "/" + "ID-";
}
+ //If the config rootPath when use zk metadata store, it will append rootPath as the prefix of the path.
+ //So when we get the path from the stat, we should truncate the rootPath.
+ private String handleTheDeletePath(String path) {
+ if (store instanceof ZKMetadataStore) {
+ String rootPath = ((ZKMetadataStore) store).getRootPath();
Review Comment:
I see.
--
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] Jason918 commented on pull request #17192: [fix][broker] Fix pulsarLedgerIdGenerator can't delete index path when zk metadata store config rootPath.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #17192:
URL: https://github.com/apache/pulsar/pull/17192#issuecomment-1223634420
> This problem may lead zk oom if the ManagedLedgerImpl create new ledger too frequent.
You can add this to the Motivation.
--
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] horizonzy commented on a diff in pull request #17192: [fix][broker] Fix pulsarLedgerIdGenerator can't delete index path when zk metadata store config rootPath.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #17192:
URL: https://github.com/apache/pulsar/pull/17192#discussion_r950791872
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerIdGenerator.java:
##########
@@ -173,7 +174,7 @@ private CompletableFuture<Long> generateLongLedgerId() {
if (log.isDebugEnabled()) {
log.debug("DELETING HIGH ORDER DIR: {}", path);
}
- store.delete(path, Optional.of(0L));
Review Comment:
Yes, you are right. Thanks for reminder.
--
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] Technoboy- commented on a diff in pull request #17192: [fix][broker] Fix pulsarLedgerIdGenerator can't delete index path when zk metadata store config rootPath.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #17192:
URL: https://github.com/apache/pulsar/pull/17192#discussion_r950966421
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerIdGenerator.java:
##########
@@ -287,4 +288,16 @@ private static String createLedgerPrefix(String ledgersPath, String idGenZnodeNa
return ledgerIdGenPath + "/" + "ID-";
}
+ //If the config rootPath when use zk metadata store, it will append rootPath as the prefix of the path.
+ //So when we get the path from the stat, we should truncate the rootPath.
+ private String handleTheDeletePath(String path) {
+ if (store instanceof ZKMetadataStore) {
+ String rootPath = ((ZKMetadataStore) store).getRootPath();
+ if (rootPath == null) {
+ return path;
+ }
+ return path.replaceFirst(rootPath, "");
+ }
+ return path;
+ }
Review Comment:
Could we make this in the constructor to avoid calling `replaceFirst` every time?
--
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] horizonzy commented on pull request #17192: [fix][broker] Fix pulsarLedgerIdGenerator can't delete index path when zk metadata store config rootPath.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on PR #17192:
URL: https://github.com/apache/pulsar/pull/17192#issuecomment-1222061805
This problem maybe lead zk oom if the ManagedLedgerImpl create new ledger too frequently.
--
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] MarvinCai commented on a diff in pull request #17192: [fix][broker] Fix pulsarLedgerIdGenerator can't delete index path when zk metadata store config rootPath.
Posted by GitBox <gi...@apache.org>.
MarvinCai commented on code in PR #17192:
URL: https://github.com/apache/pulsar/pull/17192#discussion_r950774073
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerIdGenerator.java:
##########
@@ -173,7 +174,7 @@ private CompletableFuture<Long> generateLongLedgerId() {
if (log.isDebugEnabled()) {
log.debug("DELETING HIGH ORDER DIR: {}", path);
}
- store.delete(path, Optional.of(0L));
Review Comment:
this is a relative path we build ourself and won't have the problem of carrying root path when getting path from `stats.getPath()`?
--
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] Technoboy- merged pull request #17192: [fix][broker] Fix pulsarLedgerIdGenerator can't delete index path when zk metadata store config rootPath.
Posted by GitBox <gi...@apache.org>.
Technoboy- merged PR #17192:
URL: https://github.com/apache/pulsar/pull/17192
--
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] github-actions[bot] commented on pull request #17192: [fix][broker] Fix pulsarLedgerIdGenerator can't delete index path when zk metadata store config rootPath.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #17192:
URL: https://github.com/apache/pulsar/pull/17192#issuecomment-1221264131
@horizonzy Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
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] horizonzy commented on a diff in pull request #17192: [fix][broker] Fix pulsarLedgerIdGenerator can't delete index path when zk metadata store config rootPath.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #17192:
URL: https://github.com/apache/pulsar/pull/17192#discussion_r950985311
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerIdGenerator.java:
##########
@@ -287,4 +288,16 @@ private static String createLedgerPrefix(String ledgersPath, String idGenZnodeNa
return ledgerIdGenPath + "/" + "ID-";
}
+ //If the config rootPath when use zk metadata store, it will append rootPath as the prefix of the path.
+ //So when we get the path from the stat, we should truncate the rootPath.
+ private String handleTheDeletePath(String path) {
+ if (store instanceof ZKMetadataStore) {
+ String rootPath = ((ZKMetadataStore) store).getRootPath();
Review Comment:
> I see we already have ledgersRoot of PulsarLedgerIdGenerator constructor, it's better to use it directly to avoid such checks here.
It's not the zk rootPath, the `ledgersRoot` is `/ledegrs`.
--
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] codelipenghui commented on a diff in pull request #17192: [fix][broker] Fix pulsarLedgerIdGenerator can't delete index path when zk metadata store config rootPath.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #17192:
URL: https://github.com/apache/pulsar/pull/17192#discussion_r950979256
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerIdGenerator.java:
##########
@@ -287,4 +288,16 @@ private static String createLedgerPrefix(String ledgersPath, String idGenZnodeNa
return ledgerIdGenPath + "/" + "ID-";
}
+ //If the config rootPath when use zk metadata store, it will append rootPath as the prefix of the path.
+ //So when we get the path from the stat, we should truncate the rootPath.
+ private String handleTheDeletePath(String path) {
+ if (store instanceof ZKMetadataStore) {
+ String rootPath = ((ZKMetadataStore) store).getRootPath();
+ if (rootPath == null) {
+ return path;
+ }
+ return path.replaceFirst(rootPath, "");
+ }
+ return path;
+ }
Review Comment:
It's not a fixed path, it looks like we can't avoid replacing operation.
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerIdGenerator.java:
##########
@@ -287,4 +288,16 @@ private static String createLedgerPrefix(String ledgersPath, String idGenZnodeNa
return ledgerIdGenPath + "/" + "ID-";
}
+ //If the config rootPath when use zk metadata store, it will append rootPath as the prefix of the path.
+ //So when we get the path from the stat, we should truncate the rootPath.
+ private String handleTheDeletePath(String path) {
+ if (store instanceof ZKMetadataStore) {
+ String rootPath = ((ZKMetadataStore) store).getRootPath();
Review Comment:
And we don't need to introduce a new method in `ZKMetadataStore.java`
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerIdGenerator.java:
##########
@@ -287,4 +288,16 @@ private static String createLedgerPrefix(String ledgersPath, String idGenZnodeNa
return ledgerIdGenPath + "/" + "ID-";
}
+ //If the config rootPath when use zk metadata store, it will append rootPath as the prefix of the path.
+ //So when we get the path from the stat, we should truncate the rootPath.
+ private String handleTheDeletePath(String path) {
+ if (store instanceof ZKMetadataStore) {
+ String rootPath = ((ZKMetadataStore) store).getRootPath();
Review Comment:
I see we already have `ledgersRoot` of `PulsarLedgerIdGenerator` constructor, it's better to use it directly to avoid such checks 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] horizonzy commented on a diff in pull request #17192: [fix][broker] Fix pulsarLedgerIdGenerator can't delete index path when zk metadata store config rootPath.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #17192:
URL: https://github.com/apache/pulsar/pull/17192#discussion_r950984946
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerIdGenerator.java:
##########
@@ -287,4 +288,16 @@ private static String createLedgerPrefix(String ledgersPath, String idGenZnodeNa
return ledgerIdGenPath + "/" + "ID-";
}
+ //If the config rootPath when use zk metadata store, it will append rootPath as the prefix of the path.
+ //So when we get the path from the stat, we should truncate the rootPath.
+ private String handleTheDeletePath(String path) {
+ if (store instanceof ZKMetadataStore) {
+ String rootPath = ((ZKMetadataStore) store).getRootPath();
+ if (rootPath == null) {
+ return path;
+ }
+ return path.replaceFirst(rootPath, "");
+ }
+ return path;
+ }
Review Comment:
> It's not a fixed path, it looks like we can't avoid replacing operation.
+1
--
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] horizonzy commented on a diff in pull request #17192: [fix][broker] Fix pulsarLedgerIdGenerator can't delete index path when zk metadata store config rootPath.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #17192:
URL: https://github.com/apache/pulsar/pull/17192#discussion_r950987709
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerIdGenerator.java:
##########
@@ -287,4 +288,16 @@ private static String createLedgerPrefix(String ledgersPath, String idGenZnodeNa
return ledgerIdGenPath + "/" + "ID-";
}
+ //If the config rootPath when use zk metadata store, it will append rootPath as the prefix of the path.
+ //So when we get the path from the stat, we should truncate the rootPath.
+ private String handleTheDeletePath(String path) {
+ if (store instanceof ZKMetadataStore) {
+ String rootPath = ((ZKMetadataStore) store).getRootPath();
Review Comment:
The `metadataServiceUriStr` is `metadata-store:127.0.0.1:2181/test`, it's not a standard schema, so we can't resolve the path correctly in `AbstractMetadataDriver#resolveLedgersRootPath`.
--
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