[fix][broker] Fix pulsarLedgerIdGenerator can't delete index path when zk metadata store config rootPath. #17192
Conversation
|
@horizonzy Please provide a correct documentation label for your PR. |
github-actions
bot
added
doc-label-missing
doc-not-needed
hangc0276
requested review from
merlimat,
nicoloboschi,
Jason918,
hangc0276,
315157973,
codelipenghui and
eolivelli
hangc0276
added
release/blocker
| @@ -173,7 +174,7 @@ private CompletableFuture<Long> generateLongLedgerId() { | |||
| if (log.isDebugEnabled()) { | |||
| log.debug("DELETING HIGH ORDER DIR: {}", path); | |||
| } | |||
| store.delete(path, Optional.of(0L)); | |||
There was a problem hiding this 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()?
There was a problem hiding this comment.
Yes, you are right. Thanks for reminder.
| return path.replaceFirst(rootPath, ""); | ||
| } | ||
| return path; | ||
| } |
There was a problem hiding this comment.
Could we make this in the constructor to avoid calling replaceFirst every time?
There was a problem hiding this comment.
It's not a fixed path, it looks like we can't avoid replacing operation.
There was a problem hiding this comment.
It's not a fixed path, it looks like we can't avoid replacing operation.
+1
| return path.replaceFirst(rootPath, ""); | ||
| } | ||
| return path; | ||
| } |
There was a problem hiding this comment.
It's not a fixed path, it looks like we can't avoid replacing operation.
| //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(); |
There was a problem hiding this comment.
I see we already have ledgersRoot of PulsarLedgerIdGenerator constructor, it's better to use it directly to avoid such checks here.
There was a problem hiding this comment.
And we don't need to introduce a new method in ZKMetadataStore.java
There was a problem hiding this 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.
There was a problem hiding this 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.
| //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(); |
|
This problem may lead zk oom if the ManagedLedgerImpl create new ledger too frequent. |
Technoboy-
added
cherry-picked/branch-2.11
and removed
release/blocker
Technoboy-
force-pushed
the
zk-root-patch-problem
branch
from
789781e to
365938c
Compare
| @Test | ||
| public void testGenerateLedgerIdWithZkPrefix() throws Exception { | ||
| @Cleanup | ||
| MetadataStoreExtended store = |
There was a problem hiding this comment.
If this line is the only difference between testGenerateLedgerId and testGenerateLedgerIdWithZkPrefix, we can reuse the common codes here.
There was a problem hiding this comment.
In testGenerateLedgerIdWithZkPrefix, we need check the index path is still exists.
You can add this to the Motivation. |
…n zk metadata store config rootPath. (#17192)
…n zk metadata store config rootPath. (apache#17192) (cherry picked from commit 9445fa7) (cherry picked from commit 805f985)
(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.
This problem may lead zk oom if the ManagedLedgerImpl create new ledger too frequent.
Documentation
doc-not-needed(Please explain why)