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

Fix carbonserver render error race #442

Merged

Conversation

emadolsky
Copy link
Contributor

It is a simple fix to #436.

Whenever there are multiple render requests approaching for the
same records, and the proceeding request gets an error, waiting
requests should get an error too.

It is a bit different from #431 because listener.prepareDataProto handles the 404 case and returns an empty response which will be handled correctly when it proceeds. So handling the error case would probably be the solution.

Whenever there are multiple render requests approaching for the
same records, and the proceeding request gets an error, waiting
requests should get an error too.
@deniszh
Copy link
Member

deniszh commented Dec 10, 2021

Could you please check the fix, @bom-d-van ?

@bom-d-van
Copy link
Member

yep, overall it looks good to me.

There is a DeepSource failure, @emadolsky could you check if you can fix it?
(not sure what's going on in golangci-lint.)

@deniszh
Copy link
Member

deniszh commented Dec 10, 2021

Not sure about DeepSource - it complains about unused metric parameter - https://deepsource.io/gh/go-graphite/go-carbon/run/617e9c52-2797-47f7-90f9-e9b47844eb47/go/RVV-B0012 but lint error is clear, though - https://go-critic.com/overview.html#ifElseChain-ref
IMO replace if chain with switch case is good suggestion overall.

@emadolsky emadolsky force-pushed the fix-carbonserver-render-error-race branch from af82c3e to e8e4a89 Compare December 13, 2021 10:05
@emadolsky emadolsky force-pushed the fix-carbonserver-render-error-race branch from b848992 to 43ff486 Compare December 13, 2021 10:44
@emadolsky
Copy link
Contributor Author

The metric name variable was not used in the function. The only use could be logging it. Also we could just get rid of it. But I was not sure which to choose. What do you think?

@bom-d-van
Copy link
Member

The metric name variable was not used in the function. The only use could be logging it. Also we could just get rid of it. But I was not sure which to choose. What do you think?

logging seems good for me. approving it and re-running the jobs.

@bom-d-van
Copy link
Member

bom-d-van commented Dec 13, 2021

The CodeQL ones look like the reaction of golang linter to the log4j incident. I'm reviewing them, probably gonna mark them as won't fix or false positive. please let me know if you disagree.

@bom-d-van bom-d-van merged commit a3b9536 into go-graphite:master Dec 13, 2021
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.

None yet

3 participants