Fix Stackdriver's shutdown behavior #4358
Conversation
|
@dimovelev Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
|
@dimovelev Thank you for signing the Contributor License Agreement! |
|
Thanks for the PR! |
jonatan-ivanov
added
the
waiting for feedback
|
Oh, I see #4353 in the title, let me edit it with the description. |
jonatan-ivanov
removed
the
waiting for feedback
jonatan-ivanov
changed the title
|
@jonatan-ivanov any plans to merge that? |
And: does this only suppress the error message, or does it also solve the problem? Or more importantly: will metrics continue to be written ;) |
| @@ -138,11 +138,21 @@ public void start(ThreadFactory threadFactory) { | |||
|
|
|||
| @Override | |||
| public void stop() { | |||
| if (client != null) | |||
| client.shutdownNow(); | |||
There was a problem hiding this comment.
Removing this from here will potentially be problematic (a resource leak) if the registry is stopped and started multiple times. If it is only stopped as part of close then I think this fix works. We should have some kind of tests for this, ideally.
There was a problem hiding this comment.
- added a shutdown and close when start is called
- I could not find any emulator etc. so that automated integration tests could be done against it. I have tested it manually against my GCP environment though.
| @Override | ||
| public void stop() { | ||
| if (client != null) | ||
| client.shutdownNow(); | ||
| super.stop(); | ||
| } |
There was a problem hiding this comment.
Is it necessary to have an overridden method that just calls its super()?
If we remove this method the stop() from parent is called anyway.
There was a problem hiding this comment.
Thanks for taking a look. Indeed we don't need to override if we aren't doing anything but calling the super here. I'll take care of it in a polish commit.
There was a problem hiding this comment.
Ah, it looks like removing the override breaks binary compatibility even if it is source compatible, which @dimovelev found out when they tried to remove it and added it back.
| @@ -125,6 +125,7 @@ public void start(ThreadFactory threadFactory) { | |||
| logger.error("unable to start stackdriver, service settings are not available"); | |||
| } | |||
| else { | |||
| shutdownClientIfNecessary(true); | |||
There was a problem hiding this comment.
I don't think this is necessary if nothing is done with the client in stop. The question is: should something be done in stop or only in close? We haven't been very clear about what should happen where (no JavaDocs). I'll open a separate issue to clear that up.
It's a bit wasteful of resources to not shutdown the client when a user calls stop on the registry, but in terms of publishing it shouldn't be an issue because the scheduled call to publish won't happen after stop is called until start is called again. Looking at other implementations, we do the same in WavefrontMeterRegistry - nothing special in stop and close the client on close after super.close().
There was a problem hiding this comment.
I don't think this is necessary if nothing is done with the client in
stop.
I remembered now the issue I was pointing out in my previous review. We don't want to replace the client without shutting down the old one and making it eligible for garbage collection. Therefore having this line here should prevent that.
There was a problem hiding this comment.
As promised, I opened #4632 to track clarifying stop semantics.
…the metrics one last time. Fixes 4353
…cycles would not leak clients
|
I've merged to the |
Fixes #4353