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

NettyClientTransport: use getOrCreate() for new instances of ChannelLogger AttributeKey. #7048

Merged
merged 10 commits into from May 21, 2020

Conversation

Nexproc
Copy link
Contributor

@Nexproc Nexproc commented May 19, 2020

NettyClientTransport throws an error when the class is loaded in two separate class-loaders because of some implementation detail with the underlying AttributeKey class. Instead of just blowing up when a new ChannelLogger key can't be created, this allows multiple, isolated class-loaders in the same JVM to just get the existing key. If this causes issues, this AttributeKey should be updated to use a new UUID (or something) each time it is loaded.

Improvement: Works with isolated class-loaders.
Potential Drawback: Uses the same ChannelLogger AttributeKey object across multiple, isolated classloaders.

Fixes #6707

voidzcy and others added 2 commits May 19, 2020 13:22
…hannelLogger` `AttributeKey`.

`NettyClientTransport` throws an error when the class is loaded in two separate class-loaders because of some implementation detail with the underlying `AttributeKey` class. Instead of just blowing up when a new `ChannelLogger` key can't be created, this allows multiple, isolated class-loaders in the same JVM to just get the existing key.  If this causes issues, this `AttributeKey` should be updated to use a new UUID (or something) each time it is loaded.

Improvement: Works with isolated class-loaders.
Potential Drawback: Uses the same `ChannelLogger` `AttributeKey` object across multiple, isolated classloaders.
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 19, 2020

CLA Check
The committers are authorized under a signed CLA.

@ejona86
Copy link
Member

ejona86 commented May 19, 2020

@Nexproc, please fix the checkstyle failures reported by Travis.

@ejona86 ejona86 added the TODO:release blocker Issue/PR is important enough to delay the release. Removed after release issues resolved label May 19, 2020
@ejona86
Copy link
Member

ejona86 commented May 19, 2020

As I mention on #6707, it is probably better to delete this attribute. But I think it would be good to merge this mostly as-is for the v1.30 release and then do the nicer cleanup separately.

@ejona86 ejona86 self-requested a review May 19, 2020 22:02
@voidzcy voidzcy added this to the 1.30 milestone May 20, 2020
@voidzcy voidzcy added the TODO:backport PR needs to be backported. Removed after backport complete label May 20, 2020
reorder `final static` -> `static final`
Shortened the null check to get around coverage being upset about 1 line. This lets me avoid making this getOrCreate function public just to test whether AttributeKey works.
@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 21, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 21, 2020
@ejona86 ejona86 requested a review from creamsoup May 21, 2020 19:26
@creamsoup creamsoup merged commit 3601190 into grpc:master May 21, 2020
@creamsoup
Copy link
Contributor

merged, thanks @Nexproc

@Nexproc
Copy link
Contributor Author

Nexproc commented May 21, 2020

No problem! Thank you both, @creamsoup & @ejona86, for the quick turnaround!

@ejona86
Copy link
Member

ejona86 commented May 21, 2020

Since I realized this does not impact netty+netty-shaded, this is no longer a release blocker.

@ejona86 ejona86 removed the TODO:release blocker Issue/PR is important enough to delay the release. Removed after release issues resolved label May 21, 2020
@creamsoup creamsoup removed the TODO:backport PR needs to be backported. Removed after backport complete label May 21, 2020
dfawley pushed a commit to dfawley/grpc-java that referenced this pull request Jan 15, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'channelLogger' is already in use when updating from 1.23.x to 1.24.x
5 participants