Skip to content

Make sure only series with chunks are sent#1385

Merged
gouthamve merged 2 commits intocortexproject:masterfrom
gouthamve:dont-send-empty-series
May 13, 2019
Merged

Make sure only series with chunks are sent#1385
gouthamve merged 2 commits intocortexproject:masterfrom
gouthamve:dont-send-empty-series

Conversation

@gouthamve
Copy link
Contributor

Could fix #1354

@gouthamve gouthamve requested a review from tomwilkie May 13, 2019 13:50
@tomwilkie
Copy link
Contributor

Panic in #1354 seems to be on the chunk store path, not the ingester path.

Also, might be easier to put this check in the querier side of this interface, so you can test without rolling new ingesters?

@gouthamve gouthamve force-pushed the dont-send-empty-series branch from 85bc150 to dce1f47 Compare May 13, 2019 15:42
@gouthamve
Copy link
Contributor Author

seems to be on the chunk store path, not the ingester path.

Not really, https://github.com/cortexproject/cortex/blob/master/pkg/querier/ingester_streaming_queryable.go#L99-L103, the ingester path uses code from that file, but its still the ingester streaming path.

Also, might be easier to put this check in the querier side of this interface, so you can test without rolling new ingesters?

Done.

@gouthamve gouthamve force-pushed the dont-send-empty-series branch from dce1f47 to 00cdbdc Compare May 13, 2019 16:02
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets not commit commented-out code.

@tomwilkie
Copy link
Contributor

LGTM!

gouthamve added 2 commits May 13, 2019 22:06
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve gouthamve force-pushed the dont-send-empty-series branch from 00cdbdc to d7d4ddf Compare May 13, 2019 16:37
@gouthamve gouthamve merged commit 21a0556 into cortexproject:master May 13, 2019
@gouthamve gouthamve deleted the dont-send-empty-series branch May 13, 2019 17:59
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.

Panic in querier

2 participants