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] Refactored BK ClientFactory to return futures #22853

Merged
merged 3 commits into from
Jun 6, 2024

Conversation

merlimat
Copy link
Contributor

@merlimat merlimat commented Jun 5, 2024

Fixes #22840
Fixes #22699

Motivation

BK client creation can be blocking when it's configured with rack-awareness policy.
This, combined with per-namespace BK client policies, can lead to deadlock situations as explained in #22840

Modifications

  1. Changed the BK client factory to return a future for BK client creation
  2. Internally create the BK client in background

Documentation

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

Matching PR in forked repository

PR in forked repository:

@merlimat merlimat added this to the 3.4.0 milestone Jun 5, 2024
@merlimat merlimat self-assigned this Jun 5, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 5, 2024
@merlimat
Copy link
Contributor Author

merlimat commented Jun 5, 2024

@rdhabalia

if there is no issue with #22842 then can we merge it?

There are still issues wiht #22842:

  1. It's only address 1 usage of a blocking API that is used in multiple places as if it were non-blocking
  2. Does not solve the problem that if there's a timeout, it falls back to use default BK client, which might break the assumptions

PTAL at this PR, it's a similar change, though it brings it closer to the BK client creation, so that all the factories expose non-blocking API. I believe it's a more general solution than #22842

@rdhabalia
Copy link
Contributor

I am not sure how this can help fo fix strict affinity and deadlock issue. also, sorry to say but what we did with #22846 and #22842 was not good where we ignore actual solution without proper review and other one was merged even if that approach was incorrect. and please advise if anyone has plan to create PR so, one avoid doing duplicate efforts.

@merlimat
Copy link
Contributor Author

merlimat commented Jun 5, 2024

was not good where we ignore actual solution without proper review and other one was merged even if that approach was incorrect.

?!? It was so much ignored that you had 2 people jumping on your issue report within 2 hours, discussing multiple ways on how to best solve the issue.

I am not sure how this can help fo fix strict affinity and deadlock issue.

I think I've explained in the description and subsequent comment. It's similar to your approach, though in a more general way, it addresses all the usages where we are constructing the BK client, instead of just in one single spot.

and other one was merged even if that approach was incorrect.

I think you have not read my multiple comments on why that PR was needed.

and please advise if anyone has plan to create PR so, one avoid doing duplicate efforts.

?? You posted your PR at the same time at the issue. I didn't fully agree with your proposed solution, so it's why I'm proposing a similar, though more general, way to fix the issue.

I honestly don't understand how can you be polemic about this. It baffles me.

@rdhabalia
Copy link
Contributor

rdhabalia commented Jun 5, 2024

I didn't fully agree with your proposed solution, so it's why I'm proposing a similar, though more general, way to fix the issue.

This is something that is not good. If someone has submitted the PR with the problem statement, and if I think it needs improvement then I would suggest in the PR rather create a new PR and merge it immediately. It gives idea that no one's feedback matters and I will do what I want. and that's something unexpected happened today.

I honestly don't understand how can you be polemic about this. It baffles me.

I said what I saw. I have closed all other PRs and we can merge this one.

@merlimat merlimat merged commit d74010c into apache:master Jun 6, 2024
49 of 50 checks passed
@merlimat merlimat deleted the create-bk-client-async branch June 6, 2024 00:09
@merlimat
Copy link
Contributor Author

merlimat commented Jun 6, 2024

I'm skipping backporting to branch-3.0 because there are several conflicts.

@lhotari
Copy link
Member

lhotari commented Jun 19, 2024

I'm skipping backporting to branch-3.0 because there are several conflicts.

I cherry-picked #21426 before this to reduce the amount of conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants