You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "poorbarcode (via GitHub)" <gi...@apache.org> on 2025/07/02 19:38:09 UTC

[PR] [fix][broker]Data lost due to conflit loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

poorbarcode opened a new pull request, #24478:
URL: https://github.com/apache/pulsar/pull/24478

   ### Motivation
   
   We encountered an issue that the ledger of a topic was lost after restarting ZKs
   
   ![image](https://github.com/user-attachments/assets/73bc35e8-57e2-4aea-9270-969b3a949737)
   
   After checking brokers' logs, we found that both brokers loaded up the same topic.
   
   ![image](https://github.com/user-attachments/assets/3b1e8c32-1d1d-4c02-a572-2db9cf9cb849)
   
   And we found the following logs that printed by `broker-0`
   
   ```
   2025-06-24T14:57:27,492+0000 [metadata-store-10-1] INFO  org.apache.bookkeeper.mledger.impl.ManagedLedgerFactoryImpl - Received MetadataStore session event: SessionLost
   2025-06-24T14:58:20,273+0000 [pulsar-load-manager-1-1] WARN  org.apache.pulsar.broker.loadbalance.extensions.ExtensibleLoadManagerImpl - Found this broker:logging-broker-0:8080 has not registered yet. Trying to register it
   2025-06-24T14:58:35,067+0000 [CompletableFutureDelayScheduler] WARN  org.apache.pulsar.broker.loadbalance.extensions.channel.ServiceUnitStateChannelImpl - logging-broker-0:8080 failed to wait for owner for serviceUnit:xxx/xxx/0xca6d5a6c_0xd448fc50; Trying to return the current owner:Optional[logging-broker-0:8080]
   java.util.concurrent.TimeoutException: null
   	at java.base/java.util.concurrent.CompletableFuture$Timeout.run(Unknown Source) ~[?:?]
   	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Unknown Source) ~[?:?]
   	at java.base/java.util.concurrent.FutureTask.run(Unknown Source) ~[?:?]
   	at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(Unknown Source) ~[?:?]
   	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source) ~[?:?]
   	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source) ~[?:?]
   	at java.base/java.lang.Thread.run(Unknown Source) [?:?]
   ```
   
   ### Root cause
   - `broker-0` lost the ZK session
   - Pulsar cluster switches the topic owner from `broker-0` to `broker-2`
   - `broker-0` loses the events of bundle owner switching because it has lost the session
   - Issue: it still maintains the old value of the bundle owner, which indicates `broker-0` is the topic's owner.
   
   ### Modifications
   
   - Add new events when brokers lose the ZK session: itemOutdatedListeners
     - Broker will unload all bundles that received the event
     - Clear the cached, invalidated data of `MetadataStoreTableViewImpl` 
   - Shutdown the broker if it can not recover the bundle states after the ZK session is reestablished.
   - Keep the behavior the same as before if users do not implement the new event listener `itemOutdatedListeners`, which guarantees compatibility
   
   
   ### 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 -->
   - [x] `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: x


-- 
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]Data lost due to conflit loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#discussion_r2182422420


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateMetadataStoreTableViewImpl.java:
##########
@@ -54,13 +56,23 @@ public class ServiceUnitStateMetadataStoreTableViewImpl extends ServiceUnitState
     private ServiceUnitStateDataConflictResolver conflictResolver;
     private volatile MetadataStoreTableView<ServiceUnitStateData> tableview;
 
+    @Override
     public void start(PulsarService pulsar,
                       BiConsumer<String, ServiceUnitStateData> tailItemListener,
-                      BiConsumer<String, ServiceUnitStateData> existingItemListener)
+                      BiConsumer<String, ServiceUnitStateData> existingItemListener,
+                      BiConsumer<String, ServiceUnitStateData> itemOutdatedListeners)
             throws MetadataStoreException {
         init(pulsar);
         conflictResolver = new ServiceUnitStateDataConflictResolver();
         conflictResolver.setStorageType(MetadataStore);
+        if (!(pulsar.getLocalMetadataStore() instanceof AbstractMetadataStore)
+            && !MetadataSessionExpiredPolicy.shutdown.equals(pulsar.getConfig().getZookeeperSessionExpiredPolicy())) {
+            String errorMsg = "Your current metadata store does not support the registration of session event"
+                    + " listeners. Please set \"zookeeperSessionExpiredPolicy\" to \"SHUTDOWN\"; otherwise, you will"
+                    + " encounter the issue that messages lost because of conflicted topic loading";
+            log.error(errorMsg);
+            throw new IllegalStateException(errorMsg);
+        }

Review Comment:
   > Is it difficult to support retry policy?
   
   Yes, we do not know what the implementation of `MetadataStore` is, so we do not know how we could determine whether the stats are outdated.
   
   > This cause will cause a behavior change which they will not able to start broker with retry policy?
   
   Good point, let me modify the log level to `warning` and remove the error throwing.



-- 
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]Data lost due to conflit loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#issuecomment-3030142707

   @codelipenghui 
   
   > @poorbarcode Do you know why Ledger 1049720 gets deleted? Or is it expected to be deleted?
   
   I am not sure of the exact details, I guess there is a possibility:
   
   Since two brokers assumed it is the owner, they overwrite the cursor's metadata, then we get a wrong cursor `md-position`.


-- 
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]Data lost due to conflict loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#issuecomment-3071825032

   Thanks @heesung-sn 
   
   > In this case, brokers could see conflict ownership views, but by the "pulsar fencing logic", only one broker should serve(write to) the topic, and the other brokers should close the topic/ledger from the fenced topic / metadata conflict exception.
   > Can we confirm if pulsar fencing logic works fine?
   
   It does not work fine, because the first owner has loss notifications because of a session lost.
   
   > I think we can consider failing new lookups/ownership assignments in this situation, when the broker lost metadata connections.
   > Returning the current candidate "owner" from the local info was added to improve availability, but it appears that we better optimize for consistency.
   
   Good pointer, the current PR has fixed it. See also https://github.com/apache/pulsar/pull/24478/files#diff-191ed1df4f5804c8fd6cdaf909cca01d071a110c3e701bdc98352f99e7a92e8eR469
   
   > Now I think this metadatastore, metadatacache, metadatatableview sync logic makes the code too complicated and fragile. We could consider removing this shadow hashmap(the hashmap in the tableview) and directly using the metadata cache for the metadata store tableview. (This requires handling CompletableFuture more gracefully)
   
   The map has another useful case: we need to unload owned topics once the broker state is not fine, the map maintains which bundle is owned by the current broker.


-- 
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]Data lost due to conflict loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#issuecomment-3066688751

   Btw, when the metadata session is re-established, dont we already invalidate the cache at the MetadataCache layer? So, the ownership tableview is eventually synced/invalidated? It should periodically refresh its values, too?


-- 
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]Data lost due to conflict loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#issuecomment-3067123908

   Regarding this warn
   
   > 2025-06-24T14:58:35,067+0000 [CompletableFutureDelayScheduler] WARN  org.apache.pulsar.broker.loadbalance.extensions.channel.ServiceUnitStateChannelImpl - logging-broker-0:8080 failed to wait for owner for serviceUnit:xxx/xxx/0xca6d5a6c_0xd448fc50; Trying to return the current owner:Optional[logging-broker-0:8080]
   
   I think we can consider failing new lookups/ownership assignments in this situation, when the broker lost metadata connections.
   
   Returning the current candidate "owner" from the local info was added to improve availability, but it appears that we better optimize for consistency.


-- 
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]Data lost due to conflit loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#discussion_r2181845024


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateMetadataStoreTableViewImpl.java:
##########
@@ -69,12 +81,21 @@ public void start(PulsarService pulsar,
                 this::validateServiceUnitPath,
                 List.of(this::updateOwnedServiceUnits, tailItemListener),
                 List.of(this::updateOwnedServiceUnits, existingItemListener),
-                TimeUnit.SECONDS.toMillis(pulsar.getConfiguration().getMetadataStoreOperationTimeoutSeconds())
+                List.of(this::invalidateOwnedServiceUnits, itemOutdatedListeners),
+                true,
+                TimeUnit.SECONDS.toMillis(pulsar.getConfiguration().getMetadataStoreOperationTimeoutSeconds()),
+                t -> handleTableViewShutDownEvent(t)
         );
         tableview.start();
 
     }
 
+    protected void handleTableViewShutDownEvent(Throwable throwable) {
+        log.error("The component of load-balance, which named metadata store table view has shutdown. This Broker can"
+                    + " not work anymore, start tp shutdow,");
+        pulsar.shutdownNow();

Review Comment:
   Is there any case that we will shutdown the table view without shutdown the broker?



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateTableView.java:
##########
@@ -110,4 +118,9 @@ void start(PulsarService pulsar,
      * @throws TimeoutException
      */
     void flush(long waitDurationInMillis) throws ExecutionException, InterruptedException, TimeoutException;
+
+    /**
+     * Whether it depends on the local metadata store.
+     */
+    boolean isMetadataStoreBased();

Review Comment:
   Something like expose the internal implementation to the API level. Do we have any other way to handle this case?



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateMetadataStoreTableViewImpl.java:
##########
@@ -54,13 +56,23 @@ public class ServiceUnitStateMetadataStoreTableViewImpl extends ServiceUnitState
     private ServiceUnitStateDataConflictResolver conflictResolver;
     private volatile MetadataStoreTableView<ServiceUnitStateData> tableview;
 
+    @Override
     public void start(PulsarService pulsar,
                       BiConsumer<String, ServiceUnitStateData> tailItemListener,
-                      BiConsumer<String, ServiceUnitStateData> existingItemListener)
+                      BiConsumer<String, ServiceUnitStateData> existingItemListener,
+                      BiConsumer<String, ServiceUnitStateData> itemOutdatedListeners)
             throws MetadataStoreException {
         init(pulsar);
         conflictResolver = new ServiceUnitStateDataConflictResolver();
         conflictResolver.setStorageType(MetadataStore);
+        if (!(pulsar.getLocalMetadataStore() instanceof AbstractMetadataStore)
+            && !MetadataSessionExpiredPolicy.shutdown.equals(pulsar.getConfig().getZookeeperSessionExpiredPolicy())) {
+            String errorMsg = "Your current metadata store does not support the registration of session event"
+                    + " listeners. Please set \"zookeeperSessionExpiredPolicy\" to \"SHUTDOWN\"; otherwise, you will"
+                    + " encounter the issue that messages lost because of conflicted topic loading";
+            log.error(errorMsg);
+            throw new IllegalStateException(errorMsg);
+        }

Review Comment:
   Is it difficult to support retry policy? This cause will cause a behavior change which they will not able to start broker with retry policy?



-- 
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]Data lost due to conflit loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#discussion_r2182446373


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateTableView.java:
##########
@@ -110,4 +118,9 @@ void start(PulsarService pulsar,
      * @throws TimeoutException
      */
     void flush(long waitDurationInMillis) throws ExecutionException, InterruptedException, TimeoutException;
+
+    /**
+     * Whether it depends on the local metadata store.
+     */
+    boolean isMetadataStoreBased();

Review Comment:
   It is `ServiceUnitStateTableView.java`, not `MetadataStoreTableView`, the interface will only be used by the broker load manager, so I think it is safe



-- 
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]Data lost due to conflict loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#issuecomment-3071914820

   > The map has another useful case: we need to unload owned topics once the broker state is not fine, 
   
   In fact, the original design was when the metadata store is unstable, pulsar should behave as-is at the best effort basis(minimal impact on existing clients) This "unloading all topics when lost zk connection" seems to be a new behavior.
   
   > the map maintains which bundle is owned by the current broker.
   
   Ok. One brute-force way to sync cache and tv in this case is to re-init the tableview(refresh the tableview again when connection is reestablished.) I think you already added this logic in this 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

Re: [PR] [fix][broker]Data lost due to conflict loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#issuecomment-3067142555

   > Btw, when the metadata session is re-established, dont we already invalidate the cache at the MetadataCache layer? So, the ownership tableview is eventually synced/invalidated? It should periodically refresh its values, too?
   
   Now I think this metadatastore, metadatacache, metadatatableview sync logic makes the code too complicated and fragile. We could consider removing this shadow hashmap(the hashmap in the tableview) and directly using the metadata cache for the metadata store tableview. (This requires handling CompletableFuture more gracefully)
   


-- 
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]Data lost due to conflit loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#discussion_r2182405254


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateMetadataStoreTableViewImpl.java:
##########
@@ -69,12 +81,21 @@ public void start(PulsarService pulsar,
                 this::validateServiceUnitPath,
                 List.of(this::updateOwnedServiceUnits, tailItemListener),
                 List.of(this::updateOwnedServiceUnits, existingItemListener),
-                TimeUnit.SECONDS.toMillis(pulsar.getConfiguration().getMetadataStoreOperationTimeoutSeconds())
+                List.of(this::invalidateOwnedServiceUnits, itemOutdatedListeners),
+                true,
+                TimeUnit.SECONDS.toMillis(pulsar.getConfiguration().getMetadataStoreOperationTimeoutSeconds()),
+                t -> handleTableViewShutDownEvent(t)
         );
         tableview.start();
 
     }
 
+    protected void handleTableViewShutDownEvent(Throwable throwable) {
+        log.error("The component of load-balance, which named metadata store table view has shutdown. This Broker can"
+                    + " not work anymore, start tp shutdow,");
+        pulsar.shutdownNow();

Review Comment:
   > Is there any case that we will shutdown the table view without shutting down the broker?
   
   If other components use `TableViewImpl.java` instead of `system topic`, and the expired data can be tolerated, the broker does not need to shut down.



-- 
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]Data lost due to conflict loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#issuecomment-3043387411

   ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/24478?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   Attention: Patch coverage is `74.50980%` with `26 lines` in your changes missing coverage. Please review.
   > Project coverage is 74.22%. Comparing base [(`bbc6224`)](https://app.codecov.io/gh/apache/pulsar/commit/bbc62245c5ddba1de4b1e7cee4ab49334bc36277?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`195ad83`)](https://app.codecov.io/gh/apache/pulsar/commit/195ad8306f5947152a32fb7a2f870b50fc2a1378?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   > Report is 1182 commits behind head on master.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/24478?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [...ata/tableview/impl/MetadataStoreTableViewImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/24478?src=pr&el=tree&filepath=pulsar-metadata%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fmetadata%2Ftableview%2Fimpl%2FMetadataStoreTableViewImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLW1ldGFkYXRhL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvbWV0YWRhdGEvdGFibGV2aWV3L2ltcGwvTWV0YWRhdGFTdG9yZVRhYmxlVmlld0ltcGwuamF2YQ==) | 69.33% | [15 Missing and 8 partials :warning: ](https://app.codecov.io/gh/apache/pulsar/pull/24478?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [...xtensions/channel/ServiceUnitStateChannelImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/24478?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Floadbalance%2Fextensions%2Fchannel%2FServiceUnitStateChannelImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9leHRlbnNpb25zL2NoYW5uZWwvU2VydmljZVVuaXRTdGF0ZUNoYW5uZWxJbXBsLmphdmE=) | 72.72% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pulsar/pull/24478?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   
   <details><summary>Additional details and impacted files</summary>
   
   
   [![Impacted file tree graph](https://app.codecov.io/gh/apache/pulsar/pull/24478/graphs/tree.svg?width=650&height=150&src=pr&token=acYqCpsK9J&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)](https://app.codecov.io/gh/apache/pulsar/pull/24478?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #24478      +/-   ##
   ============================================
   + Coverage     73.57%   74.22%   +0.65%     
   - Complexity    32624    32819     +195     
   ============================================
     Files          1877     1868       -9     
     Lines        139502   145823    +6321     
     Branches      15299    16717    +1418     
   ============================================
   + Hits         102638   108244    +5606     
   - Misses        28908    28991      +83     
   - Partials       7956     8588     +632     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/pulsar/pull/24478/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/24478/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `26.70% <0.98%> (+2.11%)` | :arrow_up: |
   | [systests](https://app.codecov.io/gh/apache/pulsar/pull/24478/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `23.30% <0.00%> (-1.02%)` | :arrow_down: |
   | [unittests](https://app.codecov.io/gh/apache/pulsar/pull/24478/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.72% <74.50%> (+0.88%)` | :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 with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/24478?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...el/ServiceUnitStateMetadataStoreTableViewImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/24478?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Floadbalance%2Fextensions%2Fchannel%2FServiceUnitStateMetadataStoreTableViewImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9leHRlbnNpb25zL2NoYW5uZWwvU2VydmljZVVuaXRTdGF0ZU1ldGFkYXRhU3RvcmVUYWJsZVZpZXdJbXBsLmphdmE=) | `67.74% <100.00%> (ø)` | |
   | [...ensions/channel/ServiceUnitStateTableViewBase.java](https://app.codecov.io/gh/apache/pulsar/pull/24478?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Floadbalance%2Fextensions%2Fchannel%2FServiceUnitStateTableViewBase.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9leHRlbnNpb25zL2NoYW5uZWwvU2VydmljZVVuaXRTdGF0ZVRhYmxlVmlld0Jhc2UuamF2YQ==) | `94.87% <100.00%> (ø)` | |
   | [...ensions/channel/ServiceUnitStateTableViewImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/24478?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Floadbalance%2Fextensions%2Fchannel%2FServiceUnitStateTableViewImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9leHRlbnNpb25zL2NoYW5uZWwvU2VydmljZVVuaXRTdGF0ZVRhYmxlVmlld0ltcGwuamF2YQ==) | `73.49% <100.00%> (ø)` | |
   | [...sions/channel/ServiceUnitStateTableViewSyncer.java](https://app.codecov.io/gh/apache/pulsar/pull/24478?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Floadbalance%2Fextensions%2Fchannel%2FServiceUnitStateTableViewSyncer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9leHRlbnNpb25zL2NoYW5uZWwvU2VydmljZVVuaXRTdGF0ZVRhYmxlVmlld1N5bmNlci5qYXZh) | `69.84% <ø> (ø)` | |
   | [...xtensions/channel/ServiceUnitStateChannelImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/24478?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Floadbalance%2Fextensions%2Fchannel%2FServiceUnitStateChannelImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9leHRlbnNpb25zL2NoYW5uZWwvU2VydmljZVVuaXRTdGF0ZUNoYW5uZWxJbXBsLmphdmE=) | `86.09% <72.72%> (+0.78%)` | :arrow_up: |
   | [...ata/tableview/impl/MetadataStoreTableViewImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/24478?src=pr&el=tree&filepath=pulsar-metadata%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fmetadata%2Ftableview%2Fimpl%2FMetadataStoreTableViewImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLW1ldGFkYXRhL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvbWV0YWRhdGEvdGFibGV2aWV3L2ltcGwvTWV0YWRhdGFTdG9yZVRhYmxlVmlld0ltcGwuamF2YQ==) | `74.87% <69.33%> (ø)` | |
   
   ... and [1085 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/24478/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   </details>
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </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]Data lost due to conflict loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#issuecomment-3071839786

   > It does not work fine, because the first owner has loss notifications because of a session lost.
   
   Interesting. I thought the fencing logic is guarded by the quorum of bk, regardless of zk.
   
   
   
   


-- 
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]Data lost due to conflit loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#discussion_r2182435104


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateMetadataStoreTableViewImpl.java:
##########
@@ -54,13 +56,23 @@ public class ServiceUnitStateMetadataStoreTableViewImpl extends ServiceUnitState
     private ServiceUnitStateDataConflictResolver conflictResolver;
     private volatile MetadataStoreTableView<ServiceUnitStateData> tableview;
 
+    @Override
     public void start(PulsarService pulsar,
                       BiConsumer<String, ServiceUnitStateData> tailItemListener,
-                      BiConsumer<String, ServiceUnitStateData> existingItemListener)
+                      BiConsumer<String, ServiceUnitStateData> existingItemListener,
+                      BiConsumer<String, ServiceUnitStateData> itemOutdatedListeners)
             throws MetadataStoreException {
         init(pulsar);
         conflictResolver = new ServiceUnitStateDataConflictResolver();
         conflictResolver.setStorageType(MetadataStore);
+        if (!(pulsar.getLocalMetadataStore() instanceof AbstractMetadataStore)
+            && !MetadataSessionExpiredPolicy.shutdown.equals(pulsar.getConfig().getZookeeperSessionExpiredPolicy())) {
+            String errorMsg = "Your current metadata store does not support the registration of session event"
+                    + " listeners. Please set \"zookeeperSessionExpiredPolicy\" to \"SHUTDOWN\"; otherwise, you will"
+                    + " encounter the issue that messages lost because of conflicted topic loading";
+            log.error(errorMsg);
+            throw new IllegalStateException(errorMsg);
+        }

Review Comment:
   >> This cause will cause a behavior change which they will not able to start broker with retry policy?
   
   > Good point, let me modify the log level to warning and remove the error throwing.
   
   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

Re: [PR] [fix][broker]Data lost due to conflit loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#discussion_r2182446968


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateMetadataStoreTableViewImpl.java:
##########
@@ -69,12 +81,21 @@ public void start(PulsarService pulsar,
                 this::validateServiceUnitPath,
                 List.of(this::updateOwnedServiceUnits, tailItemListener),
                 List.of(this::updateOwnedServiceUnits, existingItemListener),
-                TimeUnit.SECONDS.toMillis(pulsar.getConfiguration().getMetadataStoreOperationTimeoutSeconds())
+                List.of(this::invalidateOwnedServiceUnits, itemOutdatedListeners),

Review Comment:
   Renamed 



-- 
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]Data lost due to conflict loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#issuecomment-3066694020

   And the original design was that pulsar brokers run as-is(best effort) when metadata connections/sessions are lost(worst case scenario). 
   
   After this change, if all brokers lose metadata connections, will all bundles be unloaded(no topics will be served)?


-- 
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]Data lost due to conflict loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#issuecomment-3072084791

   @poorbarcode I raised this PR to print warn log when they are out of sync, https://github.com/apache/pulsar/pull/24513


-- 
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]Data lost due to conflit loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#issuecomment-3029289759

   @poorbarcode Do you know why Ledger 1049720 gets deleted? Or is it expected to be deleted?


-- 
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]Data lost due to conflit loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on code in PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#discussion_r2181434250


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateMetadataStoreTableViewImpl.java:
##########
@@ -69,12 +81,21 @@ public void start(PulsarService pulsar,
                 this::validateServiceUnitPath,
                 List.of(this::updateOwnedServiceUnits, tailItemListener),
                 List.of(this::updateOwnedServiceUnits, existingItemListener),
-                TimeUnit.SECONDS.toMillis(pulsar.getConfiguration().getMetadataStoreOperationTimeoutSeconds())
+                List.of(this::invalidateOwnedServiceUnits, itemOutdatedListeners),

Review Comment:
   `itemOutdatedListeners` -> `outdatedItemListeners`



-- 
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]Data lost due to conflict loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui merged PR #24478:
URL: https://github.com/apache/pulsar/pull/24478


-- 
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]Data lost due to conflict loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#issuecomment-3066646464

   Hi guys!
   
   I think this issue probably might have revealed a bigger problem.
   
   In fact, by design, I think pulsar brokers can see temporary inconsistent ownership views because of network delay or zk session issues.
   
   In this case, brokers could see conflict ownership views, but by the "pulsar fencing logic", only one broker should serve(write to) the topic, and the other brokers should close the topic/ledger from the fenced topic / metadata conflict exception. 
   
   Can we confirm if pulsar fencing logic works fine?


-- 
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]Data lost due to conflict loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#issuecomment-3067146589

   
   Now I think this metadatastore, metadatacache, metadatatableview sync logic makes the code too complicated and fragile. We could consider removing this shadow hashmap(the hashmap in the tableview) and directly using the metadata cache for the metadata store tableview. (This requires handling CompletableFuture more gracefully)
   


-- 
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]Data lost due to conflict loaded up a topic for two brokers, when enabled ServiceUnitStateMetadataStoreTableViewImpl [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #24478:
URL: https://github.com/apache/pulsar/pull/24478#issuecomment-3067129286

   > Btw, when the metadata session is re-established, dont we already invalidate the cache at the MetadataCache layer? So, the ownership tableview is eventually synced/invalidated? It should periodically refresh its values, too?
   
   Maybe we can improve the code to sync the tableview with metadata cache better, especially when the metadata cache is invalidated all. 
   
   Or, if this additional sync logic makes the code too complicated, we could consider removing this shadow hashmap but directly using the metadata cache for the metadata store tableview. (This requires handling CompletableFuture more gracefully)
   


-- 
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