You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "BewareMyPower (via GitHub)" <gi...@apache.org> on 2024/09/12 15:43:39 UTC

[PR] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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

   ### Motivation
   
   It's observed in our test environment many times after restarting the only broker with the extensible load manager's broker registry service.
   
   ```
   2024-09-12T14:00:20,557+0000 [main] ERROR org.apache.pulsar.broker.loadbalance.extensions.ExtensibleLoadManagerImpl - Failed to start the extensible load balance and close broker registry org.apache.pulsar.broker.loadbalance.extensions.BrokerRegistryImpl@6d6c4775.
   org.apache.pulsar.broker.PulsarServerException: org.apache.pulsar.metadata.api.MetadataStoreException$LockBusyException: Resource at /loadbalance/brokers/<broker-id>:8080 is already locked
           at org.apache.pulsar.broker.loadbalance.extensions.BrokerRegistryImpl.start(BrokerRegistryImpl.java:112) ~
           at org.apache.pulsar.broker.loadbalance.extensions.ExtensibleLoadManagerImpl.start(ExtensibleLoadManagerImpl.java:370) 
           at org.apache.pulsar.broker.loadbalance.extensions.ExtensibleLoadManagerWrapper.start(ExtensibleLoadManagerWrapper.java:50) 
           at org.apache.pulsar.broker.PulsarService.startLoadManagementService(PulsarService.java:1426) 
           at org.apache.pulsar.broker.PulsarService.start(PulsarService.java:986) 
           ...
   Caused by: org.apache.pulsar.metadata.api.MetadataStoreException$LockBusyException: Resource at /loadbalance/brokers/<broker-id>:8080 is already locked
           at org.apache.pulsar.metadata.api.MetadataStoreException.unwrap(MetadataStoreException.java:186) ~
           at org.apache.pulsar.broker.loadbalance.extensions.BrokerRegistryImpl.register(BrokerRegistryImpl.java:131) ~
           at org.apache.pulsar.broker.loadbalance.extensions.BrokerRegistryImpl.start(BrokerRegistryImpl.java:110) ~
           ... 16 more
   ```
   
   The "Resource xxx is already locked" is usually caused by a `BadVersionException`. It might be a bug with `LockManager`, but there are no more information to analyze. 
   
   Actually, it's not a good use case of  `LockManager` here. The broker registry only:
   1. Put it's own broker's lookup data under the of `/loadbalance/brokers/<broker-id>` when starting (in the main thread)
   2. Delete its own broker id from when closing.
   3. Query any broker's lookup data from the `/loadbalance/brokers/<broker-id>`
   4. List all children nodes of `/loadbalance/brokers`.
   
   Currently, step 1 and 2 are performed by the distribution lock implemented by `LockManager`. However, there should be no race condition because the broker id is the unique identifier of a broker. There is no case that a broker starts or closes concurrently. In addition, it's meaningless to pass a version explicitly to the metadata store.
   
   Regarding step 3 and 4, they are simply wrapping the `MetadataCache`'s methods. They are actually never related to something like "lock" though they're named `listLocks` and `readLock`.
   
   ### Modifications
   
   - Add a `put` method to `MetadataCache`, which just calls `MetadataStore#put` with `Optional.empty()` underlying.
   - Replace `LockManager` with `MetadataCache` in `BrokerRegistryImpl`.
   
   ### 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: <!-- ENTER URL HERE -->
   
   <!--
   After opening this PR, the build in apache/pulsar will fail and instructions will
   be provided for opening a PR in the PR author's forked repository.
   
   apache/pulsar pull requests should be first tested in your own fork since the 
   apache/pulsar CI based on GitHub Actions has constrained resources and quota.
   GitHub Actions provides separate quota for pull requests that are executed in 
   a forked repository.
   
   The tests will be run in the forked repository until all PR review comments have
   been handled, the tests pass and the PR is approved by a reviewer.
   -->
   


-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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

   > Currently, step 1 and 2 are performed by the distribution lock implemented by LockManager. However, there should be no race condition because the broker id is the unique identifier of a broker. There is no case that a broker starts or closes concurrently. In addition, it's meaningless to pass a version explicitly to the metadata store.
   
   @BewareMyPower But it could happen from user's incorrect configuration that more than 1 brokers use the same broker id especially for a on-prem environment. And the comment https://github.com/apache/pulsar/pull/23297#discussion_r1757265763 is also the same concern about the lock issue. It looks like something cause the lock didn't get released even if the broker unregister itself from the metadata server. 


-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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

   > Perhaps the description could be updated to include the actual reason why we want to do this change,
   
   @lhotari Updated.
   
   >  but what is the expected behavior if user made an incorrect broker ID (same broker ID)? As I understand, only the first broker should register successfully, all the other brokers with the same broker ID should get failed?
   
   @codelipenghui I'd like to say, [undefined behavior](https://en.wikipedia.org/wiki/Undefined_behavior). Kafka's behavior is also that only one broker will succeed if multiple brokers configure the same [broker.id](https://kafka.apache.org/documentation/#brokerconfigs_broker.id). However, this behavior is never documented. It only suggests generating broker id automatically by ZooKeeper:
   
   > To avoid conflicts between ZooKeeper generated broker id's and user configured broker id's, generated broker ids start from reserved.broker.max.id + 1.
   
   Actually, in real world, only if two brokers were configured with the same `advertisedAddress` and `webServicePort` would they have the same broker id. Even if they both don't fail after the change of this PR, users can still easily found the service didn't work because the advertised address is wrong.
   
   It would be good if users can quickly know they've configured the same broker id for different brokers. However, IMO, this issue caused by misconfiguration can hardly exist in practical rather than in theory.


-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/cache/impl/MetadataCacheImpl.java:
##########
@@ -243,6 +243,17 @@ public CompletableFuture<Void> create(String path, T value) {
         return future;
     }
 
+    @Override
+    public CompletableFuture<Void> put(String path, T value) {
+        final byte[] bytes;
+        try {
+            bytes = serde.serialize(path, value);
+        } catch (IOException e) {
+            return CompletableFuture.failedFuture(e);
+        }
+        return store.put(path, bytes, Optional.empty()).thenAccept(__ -> {});

Review Comment:
   Yeah, it should call `refresh` 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

Re: [PR] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/BrokerRegistryImpl.java:
##########
@@ -122,7 +120,7 @@ public boolean isStarted() {
     public synchronized void register() throws MetadataStoreException {
         if (this.state == State.Started) {
             try {
-                this.brokerLookupDataLock = brokerLookupDataLockManager.acquireLock(keyPath(brokerId), brokerLookupData)
+                brokerLookupDataMetadataCache.put(brokerIdKeyPath, brokerLookupData, EnumSet.of(CreateOption.Ephemeral))

Review Comment:
   In https://github.com/apache/pulsar/issues/23889#issuecomment-2614188629 @Joforde shared an observation that the `CreateOption.Ephemeral` will be ignored and a persistent node will get created. @BewareMyPower @heesung-sn Please check this.



-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/api/MetadataCache.java:
##########
@@ -128,6 +130,24 @@ public interface MetadataCache<T> {
      */
     CompletableFuture<Void> create(String path, T value);
 
+    /**
+     * Create or update the value of the given path in the metadata store without version comparison.
+     * <p>
+     * This method is equivalent to
+     * {@link org.apache.pulsar.metadata.api.extended.MetadataStoreExtended#put(String, byte[], Optional, EnumSet)} or
+     * {@link MetadataStore#put(String, byte[], Optional)} if the metadata store does not support this extended API,
+     * with `Optional.empty()` as the 3rd argument. It means if the path does not exist, it will be created. If the path
+     * already exists, the new value will override the old value.
+     * </p>
+     * @param path the path of the object in the metadata store
+     * @param value the object to put in the metadata store
+     * @param options the create options if the path does not in the metadata store
+     * @return the future that indicates if this operation failed, it could fail with
+     *   {@link java.io.IOException} if the value failed to be serialized
+     *   {@link MetadataStoreException} if the metadata store operation failed
+     */
+    CompletableFuture<Void> put(String path, T value, EnumSet<CreateOption> options);

Review Comment:
   > It makes sense to me since `MetadataCache` is implemented to hide some details to process when using `MetadataStore` directly.
   
   My comment was about using `EnumSet` in interface methods. It's not a good practice in interface design to require a specific implementation. Using `EnumSet` is similar to using `ArrayList` in interface methods. 
   In this case it's not addressable since the `MetadataStoreExtended` already did that mistake and we don't want to start changing that as part of 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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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

   > Do we need to apply the same change for ModularLoadManager?
   
   @Demogorgon314 +1 to me


-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/BrokerRegistryImpl.java:
##########
@@ -122,7 +119,7 @@ public boolean isStarted() {
     public synchronized void register() throws MetadataStoreException {
         if (this.state == State.Started) {
             try {
-                this.brokerLookupDataLock = brokerLookupDataLockManager.acquireLock(keyPath(brokerId), brokerLookupData)
+                brokerLookupDataMetadataCache.put(brokerIdKeyPath, brokerLookupData)

Review Comment:
   One detail here is that the lock is an ephemeral entry in the metadata store which should get expired when the session dies or disconnects. when switching from acquireLock to put, it becomes a persistent entry in the metadata store. That might be a problem.  



-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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


-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/BrokerRegistryImpl.java:
##########
@@ -122,7 +119,7 @@ public boolean isStarted() {
     public synchronized void register() throws MetadataStoreException {
         if (this.state == State.Started) {
             try {
-                this.brokerLookupDataLock = brokerLookupDataLockManager.acquireLock(keyPath(brokerId), brokerLookupData)
+                brokerLookupDataMetadataCache.put(brokerIdKeyPath, brokerLookupData)

Review Comment:
   It's still confusing to me for why did we introduce `MetadataStoreExtended` and in which case `MetadataStore` is not a `MetadataStoreExtended`. Currently `MetadataCache` just maintains a `MetadataStore` internally. Let me read more details first.



-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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

   From another perspective, this PR resolves the case the broker exited unexpectedly (e.g. by a crash). Just like the incident we met online:
   
   ```
   Sep 12, 2024 @ 07:00:10.089	#  SIGSEGV (0xb) at pc=0x00000000000049c6, pid=1, tid=454
   ```
   
   The JVM process exited by a SIGSEGV signal and no cleanup was done, even the metadata store client was not closed. Then when the broker restarted, it would fail with `LockBusyException`. After some time, the metadata store server detected the client's session timeout and then it deleted the node. Then restarting the broker would succeed.
   
   When programming, we should handle exceptions rather than errors.
   - Unexpected crash of some other unexpected logics that skip the unregister are "exceptions": it can be recovered by ignoring the version compare, like what this PR does.
   - Misconfiguration at the beginning are "errors": it cannot be recovered until the operators realized this error and updated the correct configuration.
   
   There is another solution that we can add retry logic to `BrokerRegistryImpl#register`, here is a simple example (without using `Backoff`):
   
   ```java
               final var deadline = System.currentTimeMillis() + pulsar.getConfig().getMetadataStoreSessionTimeoutMillis();
               Throwable throwable = null;
               while (true) {
                   if (System.currentTimeMillis() > deadline) {
                       throw MetadataStoreException.unwrap(throwable == null ? new TimeoutException("Failed to register")
                               : throwable);
                   }
                   try {
                       this.brokerLookupDataLock = brokerLookupDataLockManager.acquireLock(keyPath(brokerId), brokerLookupData)
                               .get(conf.getMetadataStoreOperationTimeoutSeconds(), TimeUnit.SECONDS);
                       this.state = State.Registered;
                       break;
                   } catch (InterruptedException | ExecutionException | TimeoutException e) {
                       throwable = e;
                       try {
                           Thread.sleep(100);
                       } catch (InterruptedException ex) {
                           throw MetadataStoreException.unwrap(ex);
                       }
                   }
               }
   ```


-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/api/MetadataCache.java:
##########
@@ -128,6 +130,24 @@ public interface MetadataCache<T> {
      */
     CompletableFuture<Void> create(String path, T value);
 
+    /**
+     * Create or update the value of the given path in the metadata store without version comparison.
+     * <p>
+     * This method is equivalent to
+     * {@link org.apache.pulsar.metadata.api.extended.MetadataStoreExtended#put(String, byte[], Optional, EnumSet)} or
+     * {@link MetadataStore#put(String, byte[], Optional)} if the metadata store does not support this extended API,
+     * with `Optional.empty()` as the 3rd argument. It means if the path does not exist, it will be created. If the path
+     * already exists, the new value will override the old value.
+     * </p>
+     * @param path the path of the object in the metadata store
+     * @param value the object to put in the metadata store
+     * @param options the create options if the path does not in the metadata store
+     * @return the future that indicates if this operation failed, it could fail with
+     *   {@link java.io.IOException} if the value failed to be serialized
+     *   {@link MetadataStoreException} if the metadata store operation failed
+     */
+    CompletableFuture<Void> put(String path, T value, EnumSet<CreateOption> options);

Review Comment:
   ```suggestion
       CompletableFuture<Void> put(String path, T value, Set<CreateOption> options);
   ```
   It's better to depend on the `Set` interface instead of a particular implementation (`EnumSet`)



-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/api/MetadataCache.java:
##########
@@ -128,6 +130,24 @@ public interface MetadataCache<T> {
      */
     CompletableFuture<Void> create(String path, T value);
 
+    /**
+     * Create or update the value of the given path in the metadata store without version comparison.
+     * <p>
+     * This method is equivalent to
+     * {@link org.apache.pulsar.metadata.api.extended.MetadataStoreExtended#put(String, byte[], Optional, EnumSet)} or
+     * {@link MetadataStore#put(String, byte[], Optional)} if the metadata store does not support this extended API,
+     * with `Optional.empty()` as the 3rd argument. It means if the path does not exist, it will be created. If the path
+     * already exists, the new value will override the old value.
+     * </p>
+     * @param path the path of the object in the metadata store
+     * @param value the object to put in the metadata store
+     * @param options the create options if the path does not in the metadata store
+     * @return the future that indicates if this operation failed, it could fail with
+     *   {@link java.io.IOException} if the value failed to be serialized
+     *   {@link MetadataStoreException} if the metadata store operation failed
+     */
+    CompletableFuture<Void> put(String path, T value, EnumSet<CreateOption> options);

Review Comment:
   It makes sense to me since `MetadataCache` is implemented to hide some details to process when using `MetadataStore` directly.



-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/BrokerRegistryImpl.java:
##########
@@ -122,7 +119,7 @@ public boolean isStarted() {
     public synchronized void register() throws MetadataStoreException {
         if (this.state == State.Started) {
             try {
-                this.brokerLookupDataLock = brokerLookupDataLockManager.acquireLock(keyPath(brokerId), brokerLookupData)
+                brokerLookupDataMetadataCache.put(brokerIdKeyPath, brokerLookupData)

Review Comment:
   I added the `CreateOption` parameter now. PTAL again @lhotari 
   
   BTW, I just read the code about `MetadataStoreExtended`. This API seems not designed well. Currently, built-in implementation all implement this interface. If it does not force the 3rd party library to implement the extra interfaces, it should be documented that where are these methods be used and what will happen if they are not implemented.



-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/cache/impl/MetadataCacheImpl.java:
##########
@@ -243,6 +243,17 @@ public CompletableFuture<Void> create(String path, T value) {
         return future;
     }
 
+    @Override
+    public CompletableFuture<Void> put(String path, T value) {
+        final byte[] bytes;
+        try {
+            bytes = serde.serialize(path, value);
+        } catch (IOException e) {
+            return CompletableFuture.failedFuture(e);
+        }
+        return store.put(path, bytes, Optional.empty()).thenAccept(__ -> {});

Review Comment:
   `.thenAccept(__ -> refresh(path))` would be aligned how the other operations handle cache invalidation and refreshing.



-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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

   I just thought it again. Retry is not a good solution. If the broker crashed, the registered data could still exist in the metadata store so that this "dead" broker could be selected as the owner. However, even after it restarted, it could not serve any request until the metadata store session timeout. So many topics could be unavailable for a long time.
   
   Therefore, it does not make sense to keep the current flaky logic just to fail fast for a misconfiguration case. @codelipenghui 


-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/BrokerRegistryImpl.java:
##########
@@ -122,7 +119,7 @@ public boolean isStarted() {
     public synchronized void register() throws MetadataStoreException {
         if (this.state == State.Started) {
             try {
-                this.brokerLookupDataLock = brokerLookupDataLockManager.acquireLock(keyPath(brokerId), brokerLookupData)
+                brokerLookupDataMetadataCache.put(brokerIdKeyPath, brokerLookupData)

Review Comment:
   Good point! I will change the `put` API to accept `CreateOption`



-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/BrokerRegistryImpl.java:
##########
@@ -122,7 +120,7 @@ public boolean isStarted() {
     public synchronized void register() throws MetadataStoreException {
         if (this.state == State.Started) {
             try {
-                this.brokerLookupDataLock = brokerLookupDataLockManager.acquireLock(keyPath(brokerId), brokerLookupData)
+                brokerLookupDataMetadataCache.put(brokerIdKeyPath, brokerLookupData, EnumSet.of(CreateOption.Ephemeral))

Review Comment:
   Will be addressed by PR #23902



-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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

   I'm not going to say we should handle the incorrect configuration from the user side, but what is the expected behavior if user made an incorrect broker ID (same broker ID)? As I understand, only the first broker should register successfully, all the other brokers with the same broker ID should get failed?


-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/api/MetadataCache.java:
##########
@@ -128,6 +130,24 @@ public interface MetadataCache<T> {
      */
     CompletableFuture<Void> create(String path, T value);
 
+    /**
+     * Create or update the value of the given path in the metadata store without version comparison.
+     * <p>
+     * This method is equivalent to
+     * {@link org.apache.pulsar.metadata.api.extended.MetadataStoreExtended#put(String, byte[], Optional, EnumSet)} or
+     * {@link MetadataStore#put(String, byte[], Optional)} if the metadata store does not support this extended API,
+     * with `Optional.empty()` as the 3rd argument. It means if the path does not exist, it will be created. If the path
+     * already exists, the new value will override the old value.
+     * </p>
+     * @param path the path of the object in the metadata store
+     * @param value the object to put in the metadata store
+     * @param options the create options if the path does not in the metadata store
+     * @return the future that indicates if this operation failed, it could fail with
+     *   {@link java.io.IOException} if the value failed to be serialized
+     *   {@link MetadataStoreException} if the metadata store operation failed
+     */
+    CompletableFuture<Void> put(String path, T value, EnumSet<CreateOption> options);

Review Comment:
   Oh it looks like `EnumSet` is already in `MetadataStoreExtended`. Not really addressable.



##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/api/MetadataCache.java:
##########
@@ -128,6 +130,24 @@ public interface MetadataCache<T> {
      */
     CompletableFuture<Void> create(String path, T value);
 
+    /**
+     * Create or update the value of the given path in the metadata store without version comparison.
+     * <p>
+     * This method is equivalent to
+     * {@link org.apache.pulsar.metadata.api.extended.MetadataStoreExtended#put(String, byte[], Optional, EnumSet)} or
+     * {@link MetadataStore#put(String, byte[], Optional)} if the metadata store does not support this extended API,
+     * with `Optional.empty()` as the 3rd argument. It means if the path does not exist, it will be created. If the path
+     * already exists, the new value will override the old value.
+     * </p>
+     * @param path the path of the object in the metadata store
+     * @param value the object to put in the metadata store
+     * @param options the create options if the path does not in the metadata store
+     * @return the future that indicates if this operation failed, it could fail with
+     *   {@link java.io.IOException} if the value failed to be serialized
+     *   {@link MetadataStoreException} if the metadata store operation failed
+     */
+    CompletableFuture<Void> put(String path, T value, EnumSet<CreateOption> options);

Review Comment:
   It's better to depend on the `Set` interface instead of a particular implementation (`EnumSet`)



-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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

   > @BewareMyPower But it could happen from user's incorrect configuration that more than 1 brokers use the same broker id especially for a on-prem environment. 
   
   If multiple brokers have the same broker id, the configuration would have multiple other problems. Wouldn't it be a valid requirement that the admin of the Pulsar system ensures that broker ids are unique?
   
   > And the comment [#23297 (comment)](https://github.com/apache/pulsar/pull/23297#discussion_r1757265763) is also the same concern about the lock issue. It looks like something cause the lock didn't get released even if the broker unregister itself from the metadata server.
   
   Yes, that seems to be a real issue that would need to be addressed in any case.
   


-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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

   ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/23298?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 `62.50000%` with `9 lines` in your changes missing coverage. Please review.
   > Project coverage is 74.56%. 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 [(`acf0321`)](https://app.codecov.io/gh/apache/pulsar/commit/acf0321b56cf3dbda2b58b4e98009d74c6dc28f8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   > Report is 578 commits behind head on master.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/23298?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [.../pulsar/metadata/cache/impl/MetadataCacheImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/23298?src=pr&el=tree&filepath=pulsar-metadata%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fmetadata%2Fcache%2Fimpl%2FMetadataCacheImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLW1ldGFkYXRhL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvbWV0YWRhdGEvY2FjaGUvaW1wbC9NZXRhZGF0YUNhY2hlSW1wbC5qYXZh) | 40.00% | [4 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/pulsar/pull/23298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [...ker/loadbalance/extensions/BrokerRegistryImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/23298?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Floadbalance%2Fextensions%2FBrokerRegistryImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9leHRlbnNpb25zL0Jyb2tlclJlZ2lzdHJ5SW1wbC5qYXZh) | 78.57% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pulsar/pull/23298?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/23298/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/23298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #23298      +/-   ##
   ============================================
   + Coverage     73.57%   74.56%   +0.99%     
   - Complexity    32624    34308    +1684     
   ============================================
     Files          1877     1927      +50     
     Lines        139502   145040    +5538     
     Branches      15299    15860     +561     
   ============================================
   + Hits         102638   108150    +5512     
   + Misses        28908    28617     -291     
   - Partials       7956     8273     +317     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/pulsar/pull/23298/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/23298/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.94% <41.66%> (+3.36%)` | :arrow_up: |
   | [systests](https://app.codecov.io/gh/apache/pulsar/pull/23298/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.69% <4.16%> (+0.37%)` | :arrow_up: |
   | [unittests](https://app.codecov.io/gh/apache/pulsar/pull/23298/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.90% <62.50%> (+1.05%)` | :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/23298?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...ker/loadbalance/extensions/BrokerRegistryImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/23298?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Floadbalance%2Fextensions%2FBrokerRegistryImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9leHRlbnNpb25zL0Jyb2tlclJlZ2lzdHJ5SW1wbC5qYXZh) | `82.07% <78.57%> (-2.69%)` | :arrow_down: |
   | [.../pulsar/metadata/cache/impl/MetadataCacheImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/23298?src=pr&el=tree&filepath=pulsar-metadata%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fmetadata%2Fcache%2Fimpl%2FMetadataCacheImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLW1ldGFkYXRhL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvbWV0YWRhdGEvY2FjaGUvaW1wbC9NZXRhZGF0YUNhY2hlSW1wbC5qYXZh) | `85.49% <40.00%> (-3.77%)` | :arrow_down: |
   
   ... and [567 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/23298/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   </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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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

   I swallowed the exception in `BrokerRegistryImpl#unregister()`.
   
   BTW, I think all synchronous close operations for components should not throw any exception. Otherwise, the following close operations will be skipped. Just add error logs for unexpected exceptions and warning logs for expected exceptions.


-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/cache/impl/MetadataCacheImpl.java:
##########
@@ -243,6 +243,17 @@ public CompletableFuture<Void> create(String path, T value) {
         return future;
     }
 
+    @Override
+    public CompletableFuture<Void> put(String path, T value) {
+        final byte[] bytes;
+        try {
+            bytes = serde.serialize(path, value);
+        } catch (IOException e) {
+            return CompletableFuture.failedFuture(e);
+        }
+        return store.put(path, bytes, Optional.empty()).thenAccept(__ -> {});

Review Comment:
   It seems that the possible already cached value should be expired or updated 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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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

   > But it could happen from user's incorrect configuration that more than 1 brokers use the same broker id especially for a on-prem environment. 
   
   I agree with Lari that we should not guarantee a certain behavior with wrong configuration. If you assumed invalid configuration could happen, what about two brokers configured different metadata stores? How can you prevent the broker with wrong metadata URL from starting?


-- 
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] [improve][broker] Register the broker to metadata store without version id compare [pulsar]

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

   > I'm not going to say we should handle the incorrect configuration from the user side, but what is the expected behavior if user made an incorrect broker ID (same broker ID)? As I understand, only the first broker should register successfully, all the other brokers with the same broker ID should get failed?
   
   In this case, one of the benefits of this change is that in the case of broker crash, that the broker can recover immediately. The session of the crashed instance will block starting of a new instance if we don't make this change. I think that it's a different matter to detect if someone has duplicate broker ids in their Pulsar cluster configuration. 


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