Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[improve][broker] Add retry for start service unit state channel (ExtensibleLoadManagerImpl only) #23230

Conversation

Demogorgon314
Copy link
Member

Motivation

When the service unit state channel fails to start, it will throw an exception, and after the PR #22977 , it will fail forever, because the pulsar broker cannot catch the exception, and it won't have any retry mechanism

Modifications

Add retry for start service unit state channel

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@Demogorgon314 Demogorgon314 self-assigned this Aug 27, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 27, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 14.28571% with 24 lines in your changes missing coverage. Please review.

Project coverage is 74.48%. Comparing base (bbc6224) to head (5989cce).
Report is 548 commits behind head on master.

Files Patch % Lines
...dbalance/extensions/ExtensibleLoadManagerImpl.java 14.28% 23 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23230      +/-   ##
============================================
+ Coverage     73.57%   74.48%   +0.91%     
- Complexity    32624    33684    +1060     
============================================
  Files          1877     1922      +45     
  Lines        139502   144784    +5282     
  Branches      15299    15837     +538     
============================================
+ Hits         102638   107842    +5204     
+ Misses        28908    28673     -235     
- Partials       7956     8269     +313     
Flag Coverage Δ
inttests 27.61% <14.28%> (+3.03%) ⬆️
systests 24.70% <0.00%> (+0.38%) ⬆️
unittests 73.84% <14.28%> (+0.99%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...dbalance/extensions/ExtensibleLoadManagerImpl.java 77.13% <14.28%> (-2.95%) ⬇️

... and 534 files with indirect coverage changes

Copy link
Member

@RobertIndie RobertIndie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One minor comment

@BewareMyPower
Copy link
Contributor

Let me merge it since there is no new response from @lhotari. Please open a new PR if you found something wrong.

@BewareMyPower BewareMyPower merged commit 8bb30a1 into apache:master Sep 4, 2024
50 of 51 checks passed
lhotari pushed a commit that referenced this pull request Sep 5, 2024
…ensibleLoadManagerImpl only) (#23230)

(cherry picked from commit 8bb30a1)
lhotari pushed a commit that referenced this pull request Sep 5, 2024
…ensibleLoadManagerImpl only) (#23230)

(cherry picked from commit 8bb30a1)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Sep 10, 2024
…ensibleLoadManagerImpl only) (apache#23230)

(cherry picked from commit 8bb30a1)
(cherry picked from commit 97fad78)
michalcukierman pushed a commit to michalcukierman/pulsar that referenced this pull request Sep 11, 2024
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Sep 12, 2024
…ensibleLoadManagerImpl only) (apache#23230)

(cherry picked from commit 8bb30a1)
(cherry picked from commit 97fad78)
BewareMyPower added a commit to BewareMyPower/pulsar that referenced this pull request Sep 12, 2024
…nel (ExtensibleLoadManagerImpl only) (apache#23230)"

This reverts commit 8bb30a1.
@lhotari lhotari added this to the 4.0.0 milestone Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants