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

[ISSUE #8053] Return SYSTEM_BUSY if PutMessageStatus is OS_PAGE_CACHE_BUSY #8054

Merged
merged 1 commit into from
May 31, 2024

Conversation

biningo
Copy link
Contributor

@biningo biningo commented Apr 23, 2024

Which Issue(s) This PR Fixes

Fixes #8053

@biningo biningo force-pushed the fix-page-cache-busy-response branch from 58e1bde to 91c4f43 Compare April 23, 2024 16:03
Copy link
Contributor

@RongtongJin RongtongJin left a comment

Choose a reason for hiding this comment

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

From another perspective, semantically returning busy is indeed more reasonable than error.

Copy link
Member

@yuz10 yuz10 left a comment

Choose a reason for hiding this comment

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

It is correct to change the response code, but we need more discussion on whether client should retry on this scenario. Personally I think the client should retry by default to avoid producing failure.

@biningo
Copy link
Contributor Author

biningo commented Apr 26, 2024

It is correct to change the response code, but we need more discussion on whether client should retry on this scenario. Personally I think the client should retry by default to avoid producing failure.

I agree with you

@cserwen
Copy link
Member

cserwen commented Apr 28, 2024

It is correct to change the response code, but we need more discussion on whether client should retry on this scenario. Personally I think the client should retry by default to avoid producing failure.

There has been a lot of discussion about whether system_busy should be retried. I personally support retrying. For details, please refer to #5845 @yuz10

@cserwen
Copy link
Member

cserwen commented May 10, 2024

@biningo merge develop to fix the ut

@biningo biningo force-pushed the fix-page-cache-busy-response branch from 91c4f43 to e148e32 Compare May 14, 2024 13:09
@biningo
Copy link
Contributor Author

biningo commented May 14, 2024

@biningo merge develop to fix the ut

okay, I have merged.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 42.88%. Comparing base (159a603) to head (e148e32).
Report is 5 commits behind head on develop.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #8054      +/-   ##
=============================================
- Coverage      42.94%   42.88%   -0.07%     
+ Complexity     10387    10369      -18     
=============================================
  Files           1270     1270              
  Lines          88694    88693       -1     
  Branches       11401    11401              
=============================================
- Hits           38092    38037      -55     
- Misses         45914    45949      +35     
- Partials        4688     4707      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@biningo
Copy link
Contributor Author

biningo commented May 15, 2024

Hi @yuz10
#5845 already merged. Please review again.

Copy link
Member

@yuz10 yuz10 left a comment

Choose a reason for hiding this comment

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

May affect old clients.

@lizhimins lizhimins merged commit 7a5ea90 into apache:develop May 31, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Producer should be retry put message if OS_PAGE_CACHE_BUSY
7 participants