You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "void-ptr974 (via GitHub)" <gi...@apache.org> on 2026/09/21 05:54:13 UTC
[PR] [fix][meta] Return LedgerExistException for duplicate ledger creation [pulsar]
void-ptr974 opened a new pull request, #26669:
URL: https://github.com/apache/pulsar/pull/26669
### Motivation
When a generated ledger ID already exists, the metadata store rejects the create-if-absent write with `BadVersionException`. The Pulsar BookKeeper metadata driver maps this to `MetadataVersionException`, but BookKeeper only generates a replacement ID when creation fails with `LedgerExistException`. A recoverable ID collision therefore aborts ledger creation.
### Modifications
- Map create-if-absent `BadVersionException` to `LedgerExistException` in `createLedgerMetadata`, preserving the cause. Keep update and delete version-conflict handling unchanged.
- Add metadata-store contract tests for duplicate creation, preservation of existing metadata, and stale-version update/delete errors.
- Add a BookKeeper integration regression that persists the first generated ID before the real create. It verifies that the client retries with another ID, can append to the new ledger, and preserves the colliding ledger metadata.
### Verifying this change
The new duplicate-create and client-retry tests failed on the unpatched code with `BKMetadataVersionException` on both Memory and real ZooKeeper (4 failures); the stale-version contract tests passed.
After the fix, all 20 test cases passed across Memory, real ZooKeeper, MockZooKeeper and RocksDB, including the existing basic read/write and ledger-recovery tests. Automatic test retries were disabled. Oxia was not run locally because Docker is unavailable.
```bash
TEST_METADATA_PROVIDERS=Memory,ZooKeeper,MockZooKeeper,RocksDB ./gradlew :pulsar-metadata:test \
--tests 'org.apache.pulsar.metadata.bookkeeper.PulsarLedgerManagerTest' \
--tests 'org.apache.pulsar.metadata.bookkeeper.EndToEndTest' \
-PtestRetryCount=0 -PtestFailFast=false --max-workers=4
```
`./gradlew quickCheck sanityCheck --max-workers=4 --console=plain` passed, covering repository-wide license/style checks and main/test source compilation.
### Does this pull request potentially affect one of the following parts:
- [ ] 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
--
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][meta] Return LedgerExistException for duplicate ledger creation [pulsar]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #26669:
URL: https://github.com/apache/pulsar/pull/26669#discussion_r4259786220
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerManager.java:
##########
@@ -139,9 +139,20 @@ public CompletableFuture<Versioned<LedgerMetadata>> createLedgerMetadata(long le
store.put(getLedgerPath(ledgerId), data, Optional.of(-1L))
.whenComplete((stat, ex) -> {
if (ex != null) {
- log.error().attr("ledgerId", ledgerId).exceptionMessage(ex)
- .log("Failed to create ledger");
- promise.completeExceptionally(mapToBkException(ex));
+ Throwable cause = FutureUtil.unwrapCompletionException(ex);
+ if (cause instanceof MetadataStoreException.BadVersionException
+ || cause instanceof MetadataStoreException.AlreadyExistsException) {
+ log.debug().attr("ledgerId", ledgerId).exceptionMessage(cause)
+ .log("Ledger metadata already exists");
+ // A create-if-absent conflict means the ledger already exists.
Review Comment:
Nit: this is right for a genuine collision, but without a creator token it can also be our own create. `ZKMetadataStore` retries batched ops one by one after `CONNECTIONLOSS`, so a create that committed before the response was lost comes back as `NODEEXISTS` → `BadVersionException`. `LedgerCreateOp` then moves to a new id and leaves the first id's metadata behind. That's the same orphan as before this PR, so I don't think it should block, but could the comment say so and point at #26793? Otherwise a reader will assume a conflict always means another creator owns the id. For example:
```java
// The ledger path already exists: either a genuine ID collision, or our own create that
// committed before a lost response was retried. Without a creator token (#26793) both are
// reported as LedgerExistException, so BookKeeper allocates a new ID.
```
##########
pulsar-metadata/src/test/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerManagerTest.java:
##########
@@ -0,0 +1,159 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.metadata.bookkeeper;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.UUID;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.function.Supplier;
+import org.apache.bookkeeper.client.BKException;
+import org.apache.bookkeeper.client.LedgerMetadataBuilder;
+import org.apache.bookkeeper.client.api.DigestType;
+import org.apache.bookkeeper.client.api.LedgerMetadata;
+import org.apache.bookkeeper.net.BookieId;
+import org.apache.pulsar.metadata.BaseMetadataStoreTest;
+import org.apache.pulsar.metadata.api.MetadataStore;
+import org.apache.pulsar.metadata.api.MetadataStoreConfig;
+import org.apache.pulsar.metadata.api.MetadataStoreException;
+import org.apache.pulsar.metadata.api.extended.MetadataStoreExtended;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+public class PulsarLedgerManagerTest extends BaseMetadataStoreTest {
+
+ @DataProvider(name = "createConflicts")
+ public Object[][] createConflicts() {
+ List<Object[]> cases = new ArrayList<>();
+ for (Throwable cause : List.of(new MetadataStoreException.BadVersionException("Ledger exists"),
+ new MetadataStoreException.AlreadyExistsException("Ledger exists"))) {
+ cases.add(new Object[]{cause, cause});
+ cases.add(new Object[]{new CompletionException(cause), cause});
+ cases.add(new Object[]{new ExecutionException(cause), cause});
+ cases.add(new Object[]{new CompletionException(new ExecutionException(cause)), cause});
+ }
+ return cases.toArray(Object[][]::new);
+ }
+
+ @Test(dataProvider = "createConflicts", timeOut = 30000)
+ public void testCreationConflictPreservesCause(Throwable failure, Throwable cause) throws Exception {
+ MetadataStore store = mock(MetadataStore.class);
+ doReturn(CompletableFuture.failedFuture(failure)).when(store)
+ .put(anyString(), any(byte[].class), eq(Optional.of(-1L)));
+ try (var manager = new PulsarLedgerManager(store, "/ledgers")) {
+ assertThatThrownBy(() -> manager.createLedgerMetadata(123L, metadata(123L, "value"))
+ .get(10, TimeUnit.SECONDS))
+ .cause().isInstanceOf(BKException.BKLedgerExistException.class)
+ .satisfies(error -> assertThat(error.getCause()).isSameAs(cause));
+ }
+ }
+
+ @DataProvider(name = "nonConflictFailures")
+ public Object[][] nonConflictFailures() {
+ List<Object[]> cases = new ArrayList<>();
+ for (Throwable cause : List.of(new MetadataStoreException("Store unavailable"),
+ new TimeoutException("Store request timed out"))) {
+ cases.add(new Object[]{cause, cause});
+ cases.add(new Object[]{new CompletionException(new ExecutionException(cause)), cause});
+ }
+ return cases.toArray(Object[][]::new);
+ }
+
+ @Test(dataProvider = "nonConflictFailures", timeOut = 30000)
+ public void testOtherCreationFailuresArePreserved(Throwable failure, Throwable cause) throws Exception {
+ MetadataStore store = mock(MetadataStore.class);
+ doReturn(CompletableFuture.failedFuture(failure)).when(store)
+ .put(anyString(), any(byte[].class), eq(Optional.of(-1L)));
+ try (var manager = new PulsarLedgerManager(store, "/ledgers")) {
+ assertThatThrownBy(() -> manager.createLedgerMetadata(123L, metadata(123L, "value"))
+ .get(10, TimeUnit.SECONDS)).cause().isSameAs(cause);
Review Comment:
Optional: `isSameAs(cause)` pins that non-conflict failures come back as raw, non-`BKException` throwables. That's today's behavior, but it doesn't match the `LedgerManager#createLedgerMetadata` contract (`ZKException` for other failures), and BK reports a raw throwable as `UnexpectedConditionException`. What this test needs to guarantee is that these failures are never classified as `LedgerExistException`, because that would make `LedgerCreateOp` keep allocating ids. Could it assert that directly, e.g. `BKException.getExceptionCode(e)` is not `LedgerExistException` and `cause` is in the cause chain? Then a later contract fix wouldn't have to rewrite the test.
##########
pulsar-metadata/src/test/java/org/apache/pulsar/metadata/bookkeeper/EndToEndTest.java:
##########
@@ -38,6 +50,51 @@
*/
@CustomLog
public class EndToEndTest extends BaseMetadataStoreTest {
+ @Test(dataProvider = "impl", timeOut = 60000)
Review Comment:
Optional: this covers the generated-id path. The other branch of `LedgerCreateOp#metadataCallback` is an explicit id (`makeAdv().withLedgerId(id)` / `createLedgerAdv(id, ...)`). There `generateLedgerId` is false, so a duplicate must reach the caller as `BKLedgerExistException` without a retry. Before this PR it was `BKMetadataVersionException`, so this is the user-visible part of the change. I checked it locally with a scratch test (create a ledger, then `makeAdv().withLedgerId(existingId).execute()`): `BKLedgerExistException` with this PR and `BKMetadataVersionException` without it, on Memory and ZooKeeper. A short case here would pin that contract.
--
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][meta] Return LedgerExistException for duplicate ledger creation [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #26669:
URL: https://github.com/apache/pulsar/pull/26669#discussion_r4254881880
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerManager.java:
##########
@@ -141,7 +141,15 @@ public CompletableFuture<Versioned<LedgerMetadata>> createLedgerMetadata(long le
if (ex != null) {
log.error().attr("ledgerId", ledgerId).exceptionMessage(ex)
.log("Failed to create ledger");
Review Comment:
Thanks, this is what I had in mind. Collisions now log at debug, and other create failures keep the error log with the unwrapped cause. Adding `AlreadyExistsException` to the same branch keeps the returned exception type unchanged. Thanks for tracking the `cToken` work in #26793. Resolving.
--
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][meta] Return LedgerExistException for duplicate ledger creation [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #26669:
URL: https://github.com/apache/pulsar/pull/26669#discussion_r4139412414
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerManager.java:
##########
@@ -141,7 +141,15 @@ public CompletableFuture<Versioned<LedgerMetadata>> createLedgerMetadata(long le
if (ex != null) {
log.error().attr("ledgerId", ledgerId).exceptionMessage(ex)
.log("Failed to create ledger");
Review Comment:
**[QUALITY] a recoverable id collision is still logged at ERROR**
Now that a collision is an expected, self-healing outcome (`LedgerCreateOp` retries with a new id), `log.error` fires before the classification below for every collision even though the create then succeeds. Consider logging the `LedgerExistException` case at debug and keeping the error for the other failures, e.g. by moving the logging after the `instanceof` check.
--
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][meta] Return LedgerExistException for duplicate ledger creation [pulsar]
Posted by "Denovo1998 (via GitHub)" <gi...@apache.org>.
Denovo1998 commented on code in PR #26669:
URL: https://github.com/apache/pulsar/pull/26669#discussion_r4123109407
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerManager.java:
##########
@@ -141,7 +141,15 @@ public CompletableFuture<Versioned<LedgerMetadata>> createLedgerMetadata(long le
if (ex != null) {
log.error().attr("ledgerId", ledgerId).exceptionMessage(ex)
.log("Failed to create ledger");
- promise.completeExceptionally(mapToBkException(ex));
+ Throwable cause = FutureUtil.unwrapCompletionException(ex);
+ if (cause instanceof MetadataStoreException.BadVersionException) {
Review Comment:
One concern here: I don't think every `BadVersionException` on this create-if-absent path necessarily means a real ledger-id collision.
`ZKMetadataStore` retries operations after `CONNECTIONLOSS`. If the original create actually reached ZooKeeper but the response was lost, the retry can see `NODEEXISTS`, which is then exposed as `BadVersionException`. With this change, `LedgerCreateOp` will treat that as a real collision and generate a new ledger id, while the metadata created by the first attempt is left behind.
BookKeeper's own `AbstractZkLedgerManager#createLedgerMetadata` handles exactly this ambiguity with `cToken` (bookkeeper#2006): on `NODEEXISTS` it reads the metadata back and treats the create as successful when the creator token matches; only a different token is considered a real collision.
Could we preserve that behavior here instead of mapping `BadVersionException` to `LedgerExistException` unconditionally? It would also be good to add a regression case for "the create succeeded but its result was ambiguous", since the current collision test only covers a genuinely pre-existing ledger.
--
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][meta] Return LedgerExistException for duplicate ledger creation [pulsar]
Posted by "void-ptr974 (via GitHub)" <gi...@apache.org>.
void-ptr974 commented on code in PR #26669:
URL: https://github.com/apache/pulsar/pull/26669#discussion_r4268504943
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerManager.java:
##########
@@ -139,9 +139,20 @@ public CompletableFuture<Versioned<LedgerMetadata>> createLedgerMetadata(long le
store.put(getLedgerPath(ledgerId), data, Optional.of(-1L))
.whenComplete((stat, ex) -> {
if (ex != null) {
- log.error().attr("ledgerId", ledgerId).exceptionMessage(ex)
- .log("Failed to create ledger");
- promise.completeExceptionally(mapToBkException(ex));
+ Throwable cause = FutureUtil.unwrapCompletionException(ex);
+ if (cause instanceof MetadataStoreException.BadVersionException
+ || cause instanceof MetadataStoreException.AlreadyExistsException) {
+ log.debug().attr("ledgerId", ledgerId).exceptionMessage(cause)
+ .log("Ledger metadata already exists");
+ // A create-if-absent conflict means the ledger already exists.
Review Comment:
Thanks for pointing out the ambiguity. Updated the comment in 51f03d1a3e1 to cover our own create after a lost response and link to #26793.
##########
pulsar-metadata/src/test/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerManagerTest.java:
##########
@@ -0,0 +1,159 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.metadata.bookkeeper;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.UUID;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.function.Supplier;
+import org.apache.bookkeeper.client.BKException;
+import org.apache.bookkeeper.client.LedgerMetadataBuilder;
+import org.apache.bookkeeper.client.api.DigestType;
+import org.apache.bookkeeper.client.api.LedgerMetadata;
+import org.apache.bookkeeper.net.BookieId;
+import org.apache.pulsar.metadata.BaseMetadataStoreTest;
+import org.apache.pulsar.metadata.api.MetadataStore;
+import org.apache.pulsar.metadata.api.MetadataStoreConfig;
+import org.apache.pulsar.metadata.api.MetadataStoreException;
+import org.apache.pulsar.metadata.api.extended.MetadataStoreExtended;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+public class PulsarLedgerManagerTest extends BaseMetadataStoreTest {
+
+ @DataProvider(name = "createConflicts")
+ public Object[][] createConflicts() {
+ List<Object[]> cases = new ArrayList<>();
+ for (Throwable cause : List.of(new MetadataStoreException.BadVersionException("Ledger exists"),
+ new MetadataStoreException.AlreadyExistsException("Ledger exists"))) {
+ cases.add(new Object[]{cause, cause});
+ cases.add(new Object[]{new CompletionException(cause), cause});
+ cases.add(new Object[]{new ExecutionException(cause), cause});
+ cases.add(new Object[]{new CompletionException(new ExecutionException(cause)), cause});
+ }
+ return cases.toArray(Object[][]::new);
+ }
+
+ @Test(dataProvider = "createConflicts", timeOut = 30000)
+ public void testCreationConflictPreservesCause(Throwable failure, Throwable cause) throws Exception {
+ MetadataStore store = mock(MetadataStore.class);
+ doReturn(CompletableFuture.failedFuture(failure)).when(store)
+ .put(anyString(), any(byte[].class), eq(Optional.of(-1L)));
+ try (var manager = new PulsarLedgerManager(store, "/ledgers")) {
+ assertThatThrownBy(() -> manager.createLedgerMetadata(123L, metadata(123L, "value"))
+ .get(10, TimeUnit.SECONDS))
+ .cause().isInstanceOf(BKException.BKLedgerExistException.class)
+ .satisfies(error -> assertThat(error.getCause()).isSameAs(cause));
+ }
+ }
+
+ @DataProvider(name = "nonConflictFailures")
+ public Object[][] nonConflictFailures() {
+ List<Object[]> cases = new ArrayList<>();
+ for (Throwable cause : List.of(new MetadataStoreException("Store unavailable"),
+ new TimeoutException("Store request timed out"))) {
+ cases.add(new Object[]{cause, cause});
+ cases.add(new Object[]{new CompletionException(new ExecutionException(cause)), cause});
+ }
+ return cases.toArray(Object[][]::new);
+ }
+
+ @Test(dataProvider = "nonConflictFailures", timeOut = 30000)
+ public void testOtherCreationFailuresArePreserved(Throwable failure, Throwable cause) throws Exception {
+ MetadataStore store = mock(MetadataStore.class);
+ doReturn(CompletableFuture.failedFuture(failure)).when(store)
+ .put(anyString(), any(byte[].class), eq(Optional.of(-1L)));
+ try (var manager = new PulsarLedgerManager(store, "/ledgers")) {
+ assertThatThrownBy(() -> manager.createLedgerMetadata(123L, metadata(123L, "value"))
+ .get(10, TimeUnit.SECONDS)).cause().isSameAs(cause);
Review Comment:
Thanks for the suggestion. Updated the assertions in 51f03d1a3e1 to exclude `LedgerExistException` and retain the original cause, allowing future wrapping.
##########
pulsar-metadata/src/test/java/org/apache/pulsar/metadata/bookkeeper/EndToEndTest.java:
##########
@@ -38,6 +50,51 @@
*/
@CustomLog
public class EndToEndTest extends BaseMetadataStoreTest {
+ @Test(dataProvider = "impl", timeOut = 60000)
Review Comment:
Thanks for highlighting this path. Added the explicit-ID regression in 51f03d1a3e1: `BKLedgerExistException`, one creation attempt, and unchanged stored metadata.
--
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][meta] Return LedgerExistException for duplicate ledger creation [pulsar]
Posted by "void-ptr974 (via GitHub)" <gi...@apache.org>.
void-ptr974 commented on code in PR #26669:
URL: https://github.com/apache/pulsar/pull/26669#discussion_r4151348883
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerManager.java:
##########
@@ -141,7 +141,15 @@ public CompletableFuture<Versioned<LedgerMetadata>> createLedgerMetadata(long le
if (ex != null) {
log.error().attr("ledgerId", ledgerId).exceptionMessage(ex)
.log("Failed to create ledger");
- promise.completeExceptionally(mapToBkException(ex));
+ Throwable cause = FutureUtil.unwrapCompletionException(ex);
+ if (cause instanceof MetadataStoreException.BadVersionException) {
Review Comment:
Thanks for catching this and for pointing me to BookKeeper's `cToken` handling. I traced the ambiguous-create path and opened #26793 to track the complete recovery work, including authoritative read-back, cache consistency, legacy metadata behavior, and the regression case you described.
I kept #26669 focused on the duplicate-create error mapping. The follow-up issue links back to this review and records the behavior both before and after this change.
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerManager.java:
##########
@@ -141,7 +141,15 @@ public CompletableFuture<Versioned<LedgerMetadata>> createLedgerMetadata(long le
if (ex != null) {
log.error().attr("ledgerId", ledgerId).exceptionMessage(ex)
.log("Failed to create ledger");
Review Comment:
Thanks for the detailed review. I fixed the logging point in `22433c1f7f2`: `BadVersionException` and `AlreadyExistsException` conflicts are now logged at DEBUG after unwrapping, while unexpected create failures remain at ERROR.
I also opened #26793 to track the `cToken` work separately, as you suggested.
--
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][meta] Return LedgerExistException for duplicate ledger creation [pulsar]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui merged PR #26669:
URL: https://github.com/apache/pulsar/pull/26669
--
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