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

Consider moving optional context out of CaptiveCoreConfig and make it a parameter of PrepareRange #3295

Closed
tamirms opened this issue Dec 18, 2020 · 1 comment

Comments

@tamirms
Copy link
Contributor

tamirms commented Dec 18, 2020

In #3278 CaptiveCoreConfig was extended to include an optional context field. The context in CaptiveCoreConfig acts as a parent context for all PrepareRange() sessions. In other words, if the parent context is cancelled then, any pending PrepareRange() / GetLedger() operation will be terminated even though Close() was not called.

In the PR @2opremio mentioned:

In general, it's not considered a good practice to store the context unless it's an ephemeral object (e.g. a request).

Normally, the context is passed to each operation so that it can be configured per operation.

With that feedback in mind, perhaps it would be better to move the context so that it's a parameter to PrepareRange() instead.

@bartekn
Copy link
Contributor

bartekn commented Dec 20, 2021

This was fixed in #3576.

@bartekn bartekn closed this as completed Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants