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

Watch SrvKeyspace instead of polling in VTGate. #1433

Merged
merged 3 commits into from
Jan 14, 2016
Merged

Watch SrvKeyspace instead of polling in VTGate. #1433

merged 3 commits into from
Jan 14, 2016

Conversation

guoliang100
Copy link
Contributor

newCtx, cancel := context.WithTimeout(context.Background(), *srvTopoTimeout)
defer cancel()

result, err := server.topoServer.GetSrvKeyspace(newCtx, cell, keyspace)
// start watching
notifications, _, err := server.topoServer.WatchSrvKeyspace(newCtx, cell, keyspace)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure this is right: won't the watch expire when newCtx expires?

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, looking at it now, maybe the WatchSrvKeyspace API is wrong: instead of the stopWatching channel that is returned, WatchSrvKeyspace should just close the notifications channel and return if the context is cancelled. That way the caller can just cancel the context to stop the watch, and that triggers the notification channel to be closed. So the go routine that waist on notifications can just range on the channel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified so the Watch call uses an empty context without timeout.
I will re-visit the API later.

@guoliang100
Copy link
Contributor Author

@alainjobart
Can you review again?

return entry.value, entry.lastError
}
entry.lastError = err
entry.lastErrorCtx = newCtx
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be ctx, the point is for the web page to show who originated the query, otherwise this newCtx is empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@guoliang100
Copy link
Contributor Author

@alainjobart
Can you take a look?

@alainjobart
Copy link
Contributor

LGTM

guoliang100 added a commit that referenced this pull request Jan 14, 2016
Watch SrvKeyspace instead of polling in VTGate.
@guoliang100 guoliang100 merged commit 8493d97 into master Jan 14, 2016
dbussink pushed a commit that referenced this pull request Jan 30, 2023
…be handled correctly (#11911) (#11922) (#1433)

* bugfix: allow predicates on top of derived tables to be handled without dependencies

Signed-off-by: Andres Taylor <andres@planetscale.com>

* stop normalizer from changing literals into bindvars

Signed-off-by: Andres Taylor <andres@planetscale.com>

Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Co-authored-by: Andres Taylor <andres@planetscale.com>

Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Co-authored-by: Andres Taylor <andres@planetscale.com>
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