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

[v21.11.x] redpanda: shutdown quota manager after kafka server is stopped #4673

Merged
merged 1 commit into from
May 27, 2022

Conversation

mmaslankaprv
Copy link
Member

Quota manager refrence is a part of request context. We need to make
sure that quota manager is available as long as the kafka server is
running. Moved quota manager initialization before waiting for Kafka
server shutdown to make sure that its reference is available for all
requests that are being processed.

(cherry picked from commit 3253995)

Quota manager refrence is a part of request context. We need to make
sure that quota manager is available as long as the kafka server is
running. Moved quota manager initialization before waiting for Kafka
server shutdown to make sure that its reference is available for all
requests that are being processed.

(cherry picked from commit 3253995)
@andrewhsu
Copy link
Member

which milestone should this PR belong to?

@andrewhsu andrewhsu added this to the v21.11.17 milestone May 11, 2022
@andrewhsu
Copy link
Member

adding to the v21.11.17 milestone so we don't lose track of this for future release

@graphcareful
Copy link
Contributor

graphcareful commented May 11, 2022

Just curious why we want to place this cherry-picked commit into a point release. Is it intended to fix something? I remember also observing a problem that necessitated this fix and coming to the realization that its only an issue if the portion of work that is done by connection_context in the background invokes quota_manager which I see nothing currently does.

Also a quota_manager ref cannot be obtained by request_context, so does this change intend to fix something else? Maybe we can change the cover letter then?

@graphcareful graphcareful self-requested a review May 11, 2022 18:49
@mmaslankaprv
Copy link
Member Author

Just curious why we want to place this cherry-picked commit into a point release. Is it intended to fix something? I remember also observing a problem that necessitated this fix and coming to the realization that its only an issue if the portion of work that is done by connection_context in the background invokes quota_manager which I see nothing currently does.

Also a quota_manager ref cannot be obtained by request_context, so does this change intend to fix something else? Maybe we can change the cover letter then?

This fixes shutdown sequence, it may lead to segmentation fault during redpanda shutdown.

@graphcareful
Copy link
Contributor

Just curious why we want to place this cherry-picked commit into a point release. Is it intended to fix something? I remember also observing a problem that necessitated this fix and coming to the realization that its only an issue if the portion of work that is done by connection_context in the background invokes quota_manager which I see nothing currently does.
Also a quota_manager ref cannot be obtained by request_context, so does this change intend to fix something else? Maybe we can change the cover letter then?

This fixes shutdown sequence, it may lead to segmentation fault during redpanda shutdown.

Ok, how though? I see that background work is protected by the connections gate as well

@mmaslankaprv
Copy link
Member Author

mmaslankaprv commented May 13, 2022

Just curious why we want to place this cherry-picked commit into a point release. Is it intended to fix something? I remember also observing a problem that necessitated this fix and coming to the realization that its only an issue if the portion of work that is done by connection_context in the background invokes quota_manager which I see nothing currently does.
Also a quota_manager ref cannot be obtained by request_context, so does this change intend to fix something else? Maybe we can change the cover letter then?

This fixes shutdown sequence, it may lead to segmentation fault during redpanda shutdown.

Ok, how though? I see that background work is protected by the connections gate as well

Before this patch quota_manager was stopped before Kafka RPC server was stopped. Connection context hold a reference to ss::sharded<quota_manager> but the sharded object is already stopped i.e. all underlying thread local instances are destroyed.

@dotnwat
Copy link
Member

dotnwat commented May 16, 2022

Needs CI failure linked to issue.

@mmaslankaprv
Copy link
Member Author

Needs CI failure linked to issue.

I am not sure if that was a CI failure we spotted that in one of the consumer clusters. It was related with the fix for #4051

@dotnwat
Copy link
Member

dotnwat commented May 19, 2022

@graphcareful what's left for approval on this PR?

@graphcareful
Copy link
Contributor

@graphcareful what's left for approval on this PR?

All good , LGTM

@mmaslankaprv
Copy link
Member Author

@graphcareful can you click Approve button

@mmaslankaprv mmaslankaprv merged commit 61698c0 into redpanda-data:v21.11.x May 27, 2022
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.

None yet

4 participants