This repository was archived by the owner on May 6, 2020. It is now read-only.
Extract ThreadContext from Context and add UntypedContext#1
Open
trask wants to merge 3 commits into
Open
Conversation
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
I recommend to start by reviewing the updated README in this PR, which tries to explain
motivation, copied here:
Propagation.io
A standalone context propagation library for Java that is focused on improving interoperability across existing context propagation APIs like gRPC Context and Reactor.
Design questions
Should the context have a thread-based "current" concept?
Having a thread-based "current" context makes sense in some environments, but doesn't make sense in others.
Reflecting that, some existing libraries like gRPC Context have this concept, while some existing libraries like Reactor do not.
So, in order to improve interoperability across existing context propagation APIs, the current proposal provides both options on top of the same immutable data structure in order to make transitioning between them efficient (see
ContextandThreadContext).Should the context have typed keys?
Having typed keys seems to be generally preferable (we are interested to learn more about use cases where they are not!).
However, some existing libraries like gRPC Context use typed keys, and some existing libraries like Reactor do not.
So, in order to improve interoperability across existing context propagation APIs, the current proposal provides both options on top of the same immutable data structure in order to make transitioning between them efficient (see
ContextandUntypedContext).[Note: there's no
UntypedThreadContextyet, we need to investigate more libraries and hear from more folks to know if this is needed.]Should this project support Java 7?
Supporting Java 7 limits some design choices, but is a requirement for both gRPC and OpenTelemetry.
Should context be an interface or an abstract class?
Since we are limited to Java 7,
Context,ThreadContext, andUntypedContexthave been implemented as abstract classes instead of interfaces so that their related static methods can be collocated in the same place, making the API surface feel smaller and improving discoverability.Alternatively, we could make them interfaces and move the static methods to corresponding util classes (e.g.
Contexts,ThreadContexts, andUntypedContexts), which would give implementations more flexibility.(End of README)
And then here are some notes that hopefully help while reviewing the code:
ContextgenerationtoThreadContext, was able to implementContextdirectly usingNodewhich improves memory and helps optimize interop between the different context implementations. My thought was thatgenerationissues are probably more of an issue when binding the context to the thread, compared to passing the context around directly.EMPTYin this class ("Referencing subclass from superclass initializer might lead to class loading deadlock") so I made this a static methodempty()instead.ThreadContextCurrentContext, but I likedThreadContextbecause it's explicit about being tied to the thread, and "current" is a little more vague, (e.g. current subscriber context in Reactor?)EMPTY/ROOTin this class. Named the methodempty()instead ofroot()sinceThreadContextextendsContext, and I wanted the method to hideContext.empty(). Also, it's not quite a "root" in the same sense that it used to be before theThreadContextsplit.Context, and thewithValues(..)methods are overridden to narrow the return type.Context. The APIs can be written againstContext, and if the API user is usingThreadContext, the user can passThreadContextdirectly to the API. And if the API callswithValueon the context that was passed in and returns a new context, the returned context will still be aThreadContext, so that when the user callsThreadContext.wrap(context)to convert the returnedContextback into aThreadContextit will essentially be a no-op.wrap(Runnable)andwrap(Callable)towrapRunnable(Runnable)andwrapCallable(Callable)so that they wouldn't look quite so similar to the staticwrap(Context)method, but maybe still not ideal.ThreadBindingDefaultThreadContextjust to keep classes smaller and more narrowly scoped (no strong opinion here).ThreadContextStorageThreadContext.Storageout into top-level class so that it doesn't take up space inThreadContext(no strong opinion here either).Context.KeyKey, this is not consistent with untyped context, which has getter, but I think this difference could be good, as it emphasizes the fundamental difference between typed and untyped contexts.ThreadContext.Key.ThreadContext.KeyContext.Keyto add convenience no-arg getter that usesThreadContext.current().These are the methods for converting between the different context types. I think the naming and location (instance vs static) could likely be improved.
ThreadContext.wrap(context)Contextbecause that felt like leaking thread binding concept into theContextclass.ThreadContext.of(Context)but not sure what the opposite "unwrap" method would be named in this case.threadContext.unwrap()context.toUntyped()wrapmethod onUntypedContext(similar toThreadContextabove), because it doesn't work for anything that implementsContext, it's only translates between very specific implementations ofContextandUntypedContext.untypedContext.toTyped()The existing gRPC Context tests have been adapted to test
ThreadContext. There are no tests yet forContextorUntypedContext, this will need to be addressed.Probably lots more stuff that I'm not thinking of or haven't noticed.
Feel free to comment on both big design issues and small implementation/doc issues. All types of feedback is welcome and needed.