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

streaming: fix a couple bugs #9745

Merged
merged 3 commits into from
Feb 9, 2021
Merged

streaming: fix a couple bugs #9745

merged 3 commits into from
Feb 9, 2021

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Feb 9, 2021

Details in the commit messages

Streaming can not be used for these queries because the near query
paramter indicates a specific sort of the results, and that sort
requires data that is not available to the client from the streaming
API.
The materializer is often reset when an error is received. By resetting
the retryWaiter we effectively never wait. The retryWaiter should only
be reset when we get an event without error. This is done in
Materializer.updateView().
@dnephin dnephin added type/bug Feature does not function as expected theme/streaming Related to Streaming connections between server and client labels Feb 9, 2021
@github-actions github-actions bot added the theme/health-checks Health Check functionality label Feb 9, 2021
@hashicorp-ci
Copy link
Contributor

🤔 Double check that this PR does not require a changelog entry in the .changelog directory. Reference

@dnephin dnephin requested review from banks, kyhavlov and a team February 9, 2021 19:01
10 minutes is the default blocking query timeout. Using the same value results in us hitting
the expired cache entry bug frequently. By extending this TTL we at least mitigate the problem.

The underlying bug still needs to be fixed.
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging February 9, 2021 19:36 Inactive
@vercel vercel bot temporarily deployed to Preview – consul February 9, 2021 19:36 Inactive
@dnephin dnephin merged commit 9aa8081 into master Feb 9, 2021
@dnephin dnephin deleted the dnephin/fix-streaming-bugs branch February 9, 2021 23:30
@hashicorp-ci
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/326013.

@hashicorp-ci
Copy link
Contributor

🍒✅ Cherry pick of commit 9aa8081 onto release/1.9.x succeeded!

hashicorp-ci pushed a commit that referenced this pull request Feb 9, 2021
useStreaming := s.agent.config.UseStreamingBackend && args.MinQueryIndex > 0
// useStreaming when a blocking query is requested, but not when the near
// query parameter is set, because that requires data only available to the server
// to sort the results.
Copy link
Member

Choose a reason for hiding this comment

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

Is this something we can actually fix later for streaming?

If a fix is possible but just not simple enough to do right away this seems like a good move to unblock folks. But if it's more that this feature is never going to be practice for streaming for some reason, I wonder if we should actually not fallback now and just make it a hard limitation of the feature that we document. I'd rather folks trying it out find out these limitations before it's GA etc. if we aren't actually going to have a fix for them.

I've not thought though if it's feasible to ever support this for streaming?

Copy link
Member

Choose a reason for hiding this comment

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

I wrote this yesterday before it was merged but forgot to actually submit it :( Sorry. Not to late though if we do decide to do something else for next release!

dizzyup pushed a commit that referenced this pull request Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/health-checks Health Check functionality theme/streaming Related to Streaming connections between server and client type/bug Feature does not function as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants