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

store,compact,sidecar: replace all usage of json.Decoder with json.Unmarshal #1173

Merged
merged 3 commits into from May 27, 2019

Conversation

tmhdgsn
Copy link

@tmhdgsn tmhdgsn commented May 23, 2019

fixes issue #1160

Changes

replaced usages of json.NewDecoder Decode, with ioutil.ReadAll/File and json.Unmarshal

Verification

been running in production for a few days, memory graphs appear marginally better fixes

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

It looks OK from the code perspective but need to check myself just how much it reduces RAM consumption. Will do that soon 😄 thanks for the PR!

@miekg miekg mentioned this pull request May 23, 2019
@GiedriusS
Copy link
Member

On my cluster the change is inconsequential:

Screenshot from 2019-05-23 17-09-10

The difference is only ~50MiB which you can attribute to the different queries which come into my cluster since they are load-balanced. My cluster has ~2TiB data so it's not a small one :(

What's the difference on your cluster with this change? Do you have any graphs?

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

The graph you shown @GiedriusS is for what component? And if store is while doing any requests? The benefit should be seen mostly during traffic to store gateway (:

However this definitely does not solve #448 IMO, only helps slightly.

IMO we should still merge this as json.Decoder should not be used when you don't need streaming indeed. LGTM and thanks for this @tmhdgsn !

@GiedriusS
Copy link
Member

It is definitely answering to queries hence the sharp-tooth pattern that you see (: that is Thanos Store, by the way. Either way, technically it is a good change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants