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] Predictable order for results of /health/service/:serviceName to mimic memdb #9247

Merged

Conversation

pierresouchay
Copy link
Contributor

This ensures the result is consitent with/witout streaming

Will partially fix #9239

…Name to mimic memdb

This ensures the result is consitent with/witout streaming

Will partially fix hashicorp#9239
@pierresouchay pierresouchay force-pushed the streaming_predictible_order_for_health branch from 7be29a8 to fb91190 Compare November 20, 2020 17:23
@dnephin dnephin added the theme/streaming Related to Streaming connections between server and client label Nov 25, 2020
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

I agree this should fix the problem. Just a couple minor suggestions for the implementation.

agent/cache-types/streaming_health_services.go Outdated Show resolved Hide resolved
agent/cache-types/streaming_health_services.go Outdated Show resolved Hide resolved
agent/cache-types/streaming_health_services_test.go Outdated Show resolved Hide resolved
@pierresouchay
Copy link
Contributor Author

@dnephin All done, thank you for the review

@pierresouchay pierresouchay force-pushed the streaming_predictible_order_for_health branch from 6af4104 to 07ba1ff Compare November 25, 2020 20:20
* Renamed `cachedHealResultSorter` into `sortCheckServiceNodes`
* Use `<` instead of `strings.Compare`
* Single line comparison in unit test
@pierresouchay pierresouchay force-pushed the streaming_predictible_order_for_health branch from 07ba1ff to 76d95fd Compare November 25, 2020 20:41
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Thank you! Looks great!

@dnephin dnephin merged commit 08b8a92 into hashicorp:master Nov 25, 2020
@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/288647.

@hashicorp-ci
Copy link
Contributor

🍒✅ Cherry pick of commit 08b8a92 onto release/1.9.x succeeded!

hashicorp-ci pushed a commit that referenced this pull request Nov 25, 2020
…er_for_health

[Streaming] Predictable order for results of /health/service/:serviceName to mimic memdb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/streaming Related to Streaming connections between server and client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Streaming] Order of results in /health is modified when streaming is enabled for cached queries
3 participants