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

Option for the delta server to override the channel capacity and avoid deadlocks #876

Conversation

lobkovilya
Copy link

Please check the context at #875.

This is quite a naive and straightforward way to solve it, but it'd work perfectly fine for us. Please let me know your thoughts, I'm happy to discuss a better solution.

…apacity when Config consists of the greater number of resource types

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
@lobkovilya lobkovilya force-pushed the feat/opt-to-override-number-of-types branch from f430cbb to 9b86535 Compare February 8, 2024 15:41
@valerian-roche
Copy link
Contributor

Hey 👋, I do believe it would solve this issue, but this raises concerns in my opinion with leaking too many internals of the server implementation. It would be very brittle to require users to set this, especially if it does not fail at startup but only in some edge cases. We might be able to do it in a more efficient way based on the current number of distinct opened watches in the given stream, or by using the old model when watches do not watch the resources with an ADS order requirement.

@lobkovilya
Copy link
Author

Hey @valerian-roche! Yeah, I agree, that the solution is far from ideal.

We might be able to do it in a more efficient way based on the current number of distinct opened watches in the given stream, or by using the old model when watches do not watch the resources with an ADS order requirement.

Do you know if something like that is already in progress or planned?

@valerian-roche
Copy link
Contributor

Hey, there is no ongoing work on the issue, but I may be able to take a stab at it soon.

@lobkovilya
Copy link
Author

Hey @valerian-roche, I'd like to take on this issue as we're blocked right now with go-control-plane upgrades. Could you please elaborate on the idea you mentioned here:

We might be able to do it in a more efficient way based on the current number of distinct opened watches in the given stream

Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale label Mar 31, 2024
Copy link

github-actions bot commented Apr 7, 2024

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot closed this Apr 7, 2024
@lukidzi
Copy link
Contributor

lukidzi commented Sep 9, 2024

Hi @valerian-roche,

Is it possible to include this simple fix, considering there are no other ongoing changes? This modification doesn't alter the current user behavior, as the default value remains the same, but it allows users to provide a custom size. Since there has been no progress on this issue and the change does not break anything, could we consider incorporating it into the codebase?

@valerian-roche
Copy link
Contributor

Hey, I'd like to not merge this. This is far more likely to create deadlocks for users if they are not correct with the value, and would be forever part of the public API. I'd much rather we address the core problem of this sized channel

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

Successfully merging this pull request may close these issues.

3 participants