streaming: fix not being able to enable streaming #10514
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
So that it is easier to detect and test when streaming is being used.
dnephin
added
type/bug
github-actions
bot
added
theme/health-checks
Contributor
|
🤔 This PR has changes in the |
dnephin
force-pushed
the
dnephin/actually-enable-streaming
branch
from
c8f6ecd to
3356dd1
Compare
dnephin
force-pushed
the
dnephin/actually-enable-streaming
branch
from
3356dd1 to
5de306d
Compare
This test is super racy (it's not just a single line).
This test also starts failing once streaming is enabled, because the
cache rate limit no longer applies to the requests in the test. The
queries use streaming instead of the cache.
This test is no longer valid, and the functionality is already well
tested by TestCacheThrottle. Instead of spending time rewriting this
test, let's remove it.
```
WARNING: DATA RACE
Read at 0x00c01de410fc by goroutine 735:
github.com/hashicorp/consul/agent.TestCacheRateLimit.func1()
/home/daniel/pers/code/consul/agent/agent_test.go:1024 +0x9af
github.com/hashicorp/consul/testrpc.WaitForTestAgent()
/home/daniel/pers/code/consul/testrpc/wait.go:99 +0x209
github.com/hashicorp/consul/agent.TestCacheRateLimit.func1()
/home/daniel/pers/code/consul/agent/agent_test.go:966 +0x1ad
testing.tRunner()
/usr/lib/go/src/testing/testing.go:1193 +0x202
Previous write at 0x00c01de410fc by goroutine 605:
github.com/hashicorp/consul/agent.TestCacheRateLimit.func1.2()
/home/daniel/pers/code/consul/agent/agent_test.go:998 +0xe9
Goroutine 735 (running) created at:
testing.(*T).Run()
/usr/lib/go/src/testing/testing.go:1238 +0x5d7
github.com/hashicorp/consul/agent.TestCacheRateLimit()
/home/daniel/pers/code/consul/agent/agent_test.go:961 +0x375
testing.tRunner()
/usr/lib/go/src/testing/testing.go:1193 +0x202
Goroutine 605 (finished) created at:
github.com/hashicorp/consul/agent.TestCacheRateLimit.func1()
/home/daniel/pers/code/consul/agent/agent_test.go:1022 +0x91e
github.com/hashicorp/consul/testrpc.WaitForTestAgent()
/home/daniel/pers/code/consul/testrpc/wait.go:99 +0x209
github.com/hashicorp/consul/agent.TestCacheRateLimit.func1()
/home/daniel/pers/code/consul/agent/agent_test.go:966 +0x1ad
testing.tRunner()
/usr/lib/go/src/testing/testing.go:1193 +0x202
```
And add checks to all the tests that explicitly use streaming.
If a value was already available in the local view the request is considered a cache hit. If the materialized had to wait for a value, it is considered a cache miss.
dnephin
force-pushed
the
dnephin/actually-enable-streaming
branch
from
5de306d to
bc4d349
Compare
banks
approved these changes
Jun 28, 2021
Contributor
banks
left a comment
banks
left a comment
There was a problem hiding this comment.
I see, this test is no longer valid now that streaming is actually default and so cache is not being used.
Collaborator
|
🍒 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/399092. |
Collaborator
dnephin
added a commit
that referenced
this pull request
Jun 29, 2021
…aming streaming: fix not being able to enable streaming
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a new
X-Consul-Query-Backendresponse header to make it easier to determine when streaming was used for a query. Adds a check for this header to a few tests that are meant to check that streaming is being used.And finally fix the bug that was preventing streaming from being enabled on the client.
One commit is cherry-picked from #10396. It removes a test that is both very racy and also no longer valid because the requests are made via streaming, not the cache. Since the cache rate limit functionality is already tested in the appropriate package (agent/cache) it seems like we can remove this failing/flaky test.