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

Add dumb watch mechanism for Vai #277

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Conversation

tomleb
Copy link
Contributor

@tomleb tomleb commented Sep 11, 2024

Issue rancher/rancher#40773

Support watch with Vai using a debouncer

Copy link
Contributor

@moio moio left a comment

Choose a reason for hiding this comment

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

Questions:

  • will changes to resources a user does not have access to still result in the resource counter increasing?
  • do you have any plans to handle special Steve-only resources such as counters, that come with no revisions? How does watch work currently, can we keep the mechanism as-is?

cancel()
}()
// XXX: Why was this needed at all??
apiOp = apiOp.Clone().WithContext(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

This was copied over from the non-SQLite implementation of Watch, which has this line:

apiOp = apiOp.Clone().WithContext(ctx)

That seems to come from a distant past:

a9c39ef#diff-74cdf37ae3cf46d709e040c4c6e22a30e4b62586165b5f60c71f2e3f02514d98R132

The only reason I can see is we do not want to change the passed in apiOp, as it is passed by pointer. As Stores can be nested, I can see a calling Store not to expect the called Store to modify the passed apiOp to add a context to the nested Request the apiOp wraps.

So my understanding is that cloning here has the effect to have the context be applied to callees exclusively, but not callers. Whether this really prevents any bugs I am not sure.

apiOp.Namespace = partition.Namespace
if partition.All {
return s.Watch(apiOp, schema, wr)
debounceListener := newDebounceListener(5 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

Of course we will have to make sure the listener time is configurable - at least via configuration object or environment variable, in future possibly on a request-by-request basis.

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.

2 participants