fix(pkg/stash): close etcd sessions #1887
Conversation
uhthomas
force-pushed
the
1886
branch
3 times, most recently
from
ee7ac67
to
57d3172
Compare
|
I've also excluded |
|
Question for reviewers: should we also remove the error check from |
|
I've made the changes inline with my previous comment, I hope this is okay 😄 |
| } | ||
| defer func() { |
There was a problem hiding this comment.
I prefer keeping it the way it was.
https://go.dev/play/p/cq0DGy9OJES
Knowing if unlock / session close fails is a useful thing
There was a problem hiding this comment.
The reason I made this change is because it seems a bit strange to fail the request because the mutex failed to unlock. There's not really any action to be taken, and everything else has already succeeded. The best thing we can do here is log, not fail the entire request - as it doesn't matter.
There was a problem hiding this comment.
I agree; I think we should definitely log. Can we have some tests if possible please
There was a problem hiding this comment.
@manugupt1 I've replaced the etcd tests. They should now be a lot more realistic and actually connects to an etcd server. The tests cover the main code paths and ensure that a lease is acquired, and then freed again.
There was a problem hiding this comment.
Sorry for asking again, can we log if unlock or session close fails?
There was a problem hiding this comment.
@manugupt1 There doesn't really seem to be a standard for logging in this repository. What should I use?
There was a problem hiding this comment.
I also am not entirely sure it's worth logging. It could be useful? Though neither of these conditions should have any long-lasting negative effects. If the connection to etcd is unhealthy then the lease will expire automatically with no hard done.
There was a problem hiding this comment.
@manugupt1 What are your thoughts? I'm also keen to merge this one as it is quite a major issue for the etcd integration.
I am sort of surprised this issue wasn't raised before - I wonder how many people use Athens with etcd.
There was a problem hiding this comment.
I prefer having at least a metric being emitted for things like these, I am not a user; so I will trust you!
|
We deployed this change yesterday and have seen significant improvement in both Athens and etcd. etcd performance
gRPC filtered by LeaseRevoke: 
Resource usageAthens
etcd
|
Athens creates a new session for each 'Stash' request and then never closes it. The sessions (etcd leases) are consequently held forever and eventually leads to resource exhaustion on the etcd cluster. This has been fixed by closing the acquired session at the end of the request. Fixes: gomods#1886
What is the problem I am trying to address?
Athens creates a new session for each 'Stash' request and then never closes it. The sessions (etcd leases) are consequently held forever and eventually leads to resource exhaustion on the etcd cluster.
How is the fix applied?
This has been fixed by closing the acquired session at the end of the request.
What GitHub issue(s) does this PR fix or close?
Fixes: #1886