core: split Context into a separate grpc-context artifact. #2226
Conversation
|
@zhangkun83 LGTM |
| description = 'gRPC: Context' | ||
|
|
||
| dependencies { | ||
| compile libraries.guava, |
There was a problem hiding this comment.
If this doesn't have a strict need for guava, it will be easier to reuse in other projects. can be a follow-up
There was a problem hiding this comment.
yeah looking at the code it would be trivial to move both of these to test deps. Things like context are best as dependency-free libraries.
There was a problem hiding this comment.
That sounds reasonable. I can update in this PR.
There was a problem hiding this comment.
@adriancole, I didn't say anything during review, but I was already thinking we'd do a follow-up to remove the dep :). Now works, too.
There was a problem hiding this comment.
I'm very excited about this. It would be great to share a context for
zipkin, grpc and census (and whatever else)
On Fri, Sep 2, 2016 at 8:46 AM, Eric Anderson notifications@github.com
wrote:
In context/build.gradle
#2226 (comment):@@ -0,0 +1,16 @@
+plugins {
- id "be.insaneprogramming.gradle.animalsniffer" version "1.4.0"
+}
+description = 'gRPC: Context'
+
+dependencies {
- compile libraries.guava,
@adriancole https://github.com/adriancole, I didn't say anything during
review, but I was already thinking we'd do a follow-up to remove the dep
:). Now works, too.—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/grpc/grpc-java/pull/2226/files/d41fa3dce7f0a88c99267604b55105dc5916f980#r77278670,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAD610heCwL44cNXRTKtd69gInW3vCDCks5ql3HJgaJpZM4JzN71
.
|
👍 with notes added about removing the deps. ex zipkin could easily use it if we didn't have a guava dep only to checkArgument or avoid copy/pasting a direct executor |
|
@adriancole I guess jsr305 should stay? |
|
Unless it is really required, I would remove it. Currently, zipkin
instrumentation libs have no deps, not even annotation jars. Ex we have
internal Nullable sometimes (which most tools respect). I would love to
have zero resistence using this in Brave, for example, as soon as it is out.
|
The Context API is not particularly gRPC-specific, and will be used by Census as its context propagation mechanism. Removed all dependencies to make it easy for other libraries to depend on.
zhangkun83
force-pushed
the
context_artifact
branch
from
ec8a385 to
c4f7f5c
Compare
zhangkun83
added
the
TODO:backport
|
ps when is this going to be released? |
|
"Soon." Maybe as early as this week. We're planning on back-porting it and then doing a v1.0.1 release with it. |
zhangkun83
removed
the
TODO:backport
|
@adriancole, the separate grpc-context artifact is in v1.0.1 which I just released from Sonatype. Give it some time and it should show up. |
|
sweet On Thu, Sep 15, 2016 at 7:15 AM, Eric Anderson notifications@github.com
|
|
I see an issue with splitting these jars especially when importing them into OSGI. The grpc-core.jar and grpc-context.jar seem to export the same package which doesnt really go well with OSGI. Is it possible to have a unified artifact which would be usable in OSGI context be available? |
The Context API is not particularly gRPC-specific, and will be used by Census as its context propagation mechanism.