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 2024/07/09 17:40:25 UTC

[PR] [improve] [test] Add a test: topic will be unloaded after metadata store session rebuilt [pulsar]

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

   ### Motivation & Modifications
   
   Add a test to confirm the topic will be unloaded after the metadata store session is rebuilt.
   - Broker load a topic up.
   - The ZK session expires.
   - The ZK session rebuild.
   - Verify: The topics that the broker owned will be unloaded
   
   ### 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] Internal reader of __change_events can not started after metadata store session rebuilt [pulsar]

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##########
@@ -496,6 +496,17 @@ public void registerSessionListener(Consumer<SessionEvent> listener) {
 
     protected void receivedSessionEvent(SessionEvent event) {
         isConnected = event.isConnected();
+
+        // Clear cache after session expired.

Review Comment:
   Would it make sense to do it after new session is re-established, so that we would be able to continue with what we have in cache.



-- 
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] Internal reader of __change_events can not started after metadata store session rebuilt [pulsar]

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##########
@@ -496,6 +496,17 @@ public void registerSessionListener(Consumer<SessionEvent> listener) {
 
     protected void receivedSessionEvent(SessionEvent event) {
         isConnected = event.isConnected();
+
+        // Clear cache after session expired.

Review Comment:
   > Even though the session is re-established, it may lose some notifications, so clearing the cache is better.
   
   Yes, I was just thinking to do the clearing, though at the time of reconnection, not when the session has expired.



-- 
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] Internal reader of __change_events can not started after metadata store session rebuilt [pulsar]

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##########
@@ -496,6 +496,17 @@ public void registerSessionListener(Consumer<SessionEvent> listener) {
 
     protected void receivedSessionEvent(SessionEvent event) {
         isConnected = event.isConnected();
+
+        // Clear cache after session expired.

Review Comment:
   Good pointer, it has been fixed. Thanks 



-- 
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] Internal reader of __change_events can not started after metadata store session rebuilt [pulsar]

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##########
@@ -498,7 +498,7 @@ protected void receivedSessionEvent(SessionEvent event) {
         isConnected = event.isConnected();
 
         // Clear cache after session expired.
-        if (event == SessionEvent.SessionLost || event == SessionEvent.ConnectionLost) {
+        if (event == SessionEvent.SessionReestablished) {

Review Comment:
   please also add `SessionEvent.Reconnected` 



-- 
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] Internal reader of __change_events can not started after metadata store session rebuilt [pulsar]

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##########
@@ -496,6 +496,17 @@ public void registerSessionListener(Consumer<SessionEvent> listener) {
 
     protected void receivedSessionEvent(SessionEvent event) {
         isConnected = event.isConnected();
+
+        // Clear cache after session expired.

Review Comment:
   @lhotari @merlimat 
   
   Sorry, I missunderstood before, it has been fixed now



-- 
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] [test] Add a test: topic will be unloaded after metadata store session rebuilt [pulsar]

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

   Can we cover the new load balancer behavior 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] Internal reader of __change_events can not started after metadata store session rebuilt [pulsar]

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##########
@@ -498,7 +498,7 @@ protected void receivedSessionEvent(SessionEvent event) {
         isConnected = event.isConnected();
 
         // Clear cache after session expired.
-        if (event == SessionEvent.SessionLost || event == SessionEvent.ConnectionLost) {
+        if (event == SessionEvent.SessionReestablished) {

Review Comment:
   Sure, added



-- 
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] [test] Add a test: topic will be unloaded after metadata store session rebuilt [pulsar]

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

   ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/23018?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   All modified and coverable lines are covered by tests :white_check_mark:
   > Project coverage is 73.42%. 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 [(`efe80ba`)](https://app.codecov.io/gh/apache/pulsar/commit/efe80bacfb7cd436f1c29c975b6fd0471944c3f6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   > Report is 445 commits behind head on master.
   
   <details><summary>Additional details and impacted files</summary>
   
   
   [![Impacted file tree graph](https://app.codecov.io/gh/apache/pulsar/pull/23018/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/23018?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #23018      +/-   ##
   ============================================
   - Coverage     73.57%   73.42%   -0.15%     
   - Complexity    32624    33342     +718     
   ============================================
     Files          1877     1913      +36     
     Lines        139502   143384    +3882     
     Branches      15299    15637     +338     
   ============================================
   + Hits         102638   105286    +2648     
   - Misses        28908    30047    +1139     
   - Partials       7956     8051      +95     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/pulsar/pull/23018/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/23018/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.55% <ø> (+2.97%)` | :arrow_up: |
   | [systests](https://app.codecov.io/gh/apache/pulsar/pull/23018/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.75% <ø> (+0.42%)` | :arrow_up: |
   | [unittests](https://app.codecov.io/gh/apache/pulsar/pull/23018/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.48% <ø> (-0.36%)` | :arrow_down: |
   
   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](https://app.codecov.io/gh/apache/pulsar/pull/23018?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...roker/loadbalance/impl/ModularLoadManagerImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/23018?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Floadbalance%2Fimpl%2FModularLoadManagerImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9pbXBsL01vZHVsYXJMb2FkTWFuYWdlckltcGwuamF2YQ==) | `85.94% <ø> (+0.96%)` | :arrow_up: |
   
   ... and [482 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/23018/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] [fix] [broker] Internal reader of __change_events can not started after metadata store session rebuilt [pulsar]

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##########
@@ -496,6 +496,17 @@ public void registerSessionListener(Consumer<SessionEvent> listener) {
 
     protected void receivedSessionEvent(SessionEvent event) {
         isConnected = event.isConnected();
+
+        // Clear cache after session expired.
+        if (event == SessionEvent.SessionLost || event == SessionEvent.ConnectionLost) {
+            for (MetadataCacheImpl metadataCache : metadataCaches) {
+                metadataCache.invalidateAll();
+            }
+            childrenCache.synchronous().invalidateAll();
+            existsCache.synchronous().invalidateAll();

Review Comment:
   call `invalidateAll` instead



-- 
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] Internal reader of __change_events can not started after metadata store session rebuilt [pulsar]

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##########
@@ -496,6 +496,17 @@ public void registerSessionListener(Consumer<SessionEvent> listener) {
 
     protected void receivedSessionEvent(SessionEvent event) {
         isConnected = event.isConnected();
+
+        // Clear cache after session expired.
+        if (event == SessionEvent.SessionLost || event == SessionEvent.ConnectionLost) {
+            for (MetadataCacheImpl metadataCache : metadataCaches) {
+                metadataCache.invalidateAll();
+            }
+            childrenCache.synchronous().invalidateAll();
+            existsCache.synchronous().invalidateAll();

Review Comment:
   Fixed



-- 
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] Internal reader of __change_events can not started after metadata store session rebuilt [pulsar]

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##########
@@ -496,6 +496,17 @@ public void registerSessionListener(Consumer<SessionEvent> listener) {
 
     protected void receivedSessionEvent(SessionEvent event) {
         isConnected = event.isConnected();
+
+        // Clear cache after session expired.

Review Comment:
   > Good pointer, it has been fixed. Thanks
   
   @poorbarcode how was @merlimat's comment addressed? It might be better to postpone the flushing of caches until reconnection has finished.



-- 
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] [test] Add a test: topic will be unloaded after metadata store session rebuilt [pulsar]

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

   @heesung-sn 
   
   > Can we cover the new load balancer behavior too?
   
   Sure, I will push two following PRs to do this:
   - [ ] Enable system topic and topic-level policies. So far, this test can not pass when enabled system topic, I am checking why it does not work, and I will push a fix if there is an issue.
   - [ ] Enable `ExtensibleLoadManager`, since it depends on system topic, this test will be added after the above one.
   


-- 
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] Internal reader of __change_events can not started after metadata store session rebuilt [pulsar]

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##########
@@ -496,6 +496,17 @@ public void registerSessionListener(Consumer<SessionEvent> listener) {
 
     protected void receivedSessionEvent(SessionEvent event) {
         isConnected = event.isConnected();
+
+        // Clear cache after session expired.

Review Comment:
   @merlimat 
   
   1. Session expires
   2. ZK nodes updated
     a. **(Highlight)**: notifications will be lost and makes the cache dirty.
   3. Session re-established
   
   Even though the session is re-established, it may lose some notifications, so clearing the cache is better.



##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##########
@@ -496,6 +496,17 @@ public void registerSessionListener(Consumer<SessionEvent> listener) {
 
     protected void receivedSessionEvent(SessionEvent event) {
         isConnected = event.isConnected();
+
+        // Clear cache after session expired.

Review Comment:
   @merlimat 
   
   1. Session expires
   2. ZK nodes were updated
     a. **(Highlight)**: notifications will be lost and makes the cache dirty.
   3. Session re-established
   
   Even though the session is re-established, it may lose some notifications, so clearing the cache is better.



-- 
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] Internal reader of __change_events can not started after metadata store session rebuilt [pulsar]

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


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