Remove previously deprecated methods and make Continuation static#1419
Merged
Conversation
Contributor
|
Can we put a bit more detail in the PR description as to what the problem was and how this addresses it? |
tylerbenson
changed the title
tylerbenson
force-pushed
the
tyler/make-continuation-static
branch
5 times, most recently
from
15f5525 to
855425d
Compare
`finishSpanOnClose` will still work at the OpenTracing API level, except for `ScopeManger.active()` which returns a wrapped scope without the original `finishSpanOnClose` boolean.
The motivation here is to prevent unconstrained memory usage growth resulting from a chain of `ContinuableScope`/`Continuation` references like so:
```java
ContinuableScope scope = tracer.scopeManager().active();
Continuation continuation = scope.capture();
scope.close();
while(true){
try(Scope scope = continuation.activate()) {
continuation = scope.capture();
}
}
```
Prior to this change the above would result in an OOM error. Now, the Continuation shouldn't hold on to the previous Scope which will allow it to be GC'd.
`test continuation doesn't have hard reference on scope` is still failing, so we need to figure out where the hard reference is.
tylerbenson
force-pushed
the
tyler/make-continuation-static
branch
from
2add3ef to
17ce99b
Compare
richardstartin
approved these changes
May 13, 2020
Contributor
richardstartin
left a comment
richardstartin
left a comment
There was a problem hiding this comment.
Is there more to this than removing implicit references to outer classes? If not, looks good.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
finishSpanOnClosewill still work at the OpenTracing API level, except forScopeManger.active()which returns a wrapped scope without the originalfinishSpanOnCloseboolean.The motivation here is to prevent unconstrained memory usage growth resulting from a chain of
ContinuableScope/Continuationreferences like so:Prior to this change the above would result in an OOM error. Now, the Continuation shouldn't hold on to the previous Scope which will allow it to be GC'd.
This PR should go out in the release following #1424.