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

Added netcore namespace #33

Merged
merged 1 commit into from
Oct 23, 2017

Conversation

gideonkorir
Copy link
Contributor

Added namespaces for netcore (C# with .netcore). Namespaces are:

  1. Jaeger.Thrift
  2. Jaeger.Thrift.Agent
  3. Jaeger.Thrift.Agent.Zipkin

I did not modify the build script because at the moment I'm using an unofficial version of thrift that supports netcore

@CLAassistant
Copy link

CLAassistant commented Oct 10, 2017

CLA assistant check
All committers have signed the CLA.

@jeking3
Copy link

jeking3 commented Oct 10, 2017

Does everything work as expected with the changes for dotnet 2.0 official from the thrift pull request? I had some trouble getting the cross tests running a netcore server, but the netcore client is passing against all other thrift language servers that have common transports and protocols. I was considering merging it into thrift as there have been no objections on the mailing list.

By the way thrift already has "official" support for netcore but it is with dotnet 1.0 preview.

@gideonkorir
Copy link
Contributor Author

@jeking3 yes I managed to get the cross tests working alright (although I did have to pip install six and gevent to get the cross tests succeeding - the cpp tests were flaky every now and then). I saw the support in 1.0 but I wanted support for netstandard 2.0 because the new asp.net core libraries support netstandard 2.0 only and I was using them. In the future (assuming you merge your PR) I could help cross compile for netstandard 1.6 and 2.0.

@jeking3
Copy link

jeking3 commented Oct 10, 2017

Okay, I am going to merge this in, and any help you can provide expanding the usability to other .net core levels or in getting the servers running in cross test would be great.

@@ -24,6 +24,7 @@ include "jaeger.thrift"
include "zipkincore.thrift"

namespace java com.uber.jaeger.agent.thrift
namespace netcore Jaeger.Thrift.Agent
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since which Thrift version is this instruction supported?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

netcore is supported for dotnet-1.0.0-preview in Thrift 0.10.0
As of today, I merged in dotnet 2.0.0 official support into the thrift master which will become 0.11.0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. In this case we shouldn't merge this since most of our client libs are using thrift 0.9.

We either need to strip this string during builds of other clients, or better have it added in dotnet project only.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yurishkuro I ran the thrift files against 0.9.2 and didn't get an issue. I did not alter the Makefile because of the version bump. At the moment I'm using the changes that just got merged to Master by @jeking3 so can't add a build file for that unless I use an unofficial thrift docker container for the build

@jeking3
Copy link

jeking3 commented Oct 11, 2017

The ubuntu-xenial Dockerfile in the thrift project includes almost all languages including dotnet-2.0 official and it is what we use to build most of the thrift jobs in Travis CI.

@gideonkorir
Copy link
Contributor Author

@jeking3 the build uses the thrift:0.92 docker image. Unless I'm missing something the changes you merged into master aren't yet available as a docker image or am I wrong?

@jeking3
Copy link

jeking3 commented Oct 11, 2017

The docker image I described is the one we use to build thrift itself. I'm not sure what's in the thrift:0.92 image or where it came from.

@gideonkorir
Copy link
Contributor Author

@jeking3 from what I can make from the Makefile we are using this image to build the .thrift files. And that has up to version 0.10.0 at the time of this writing. As far as I can tell, the image has an already built thrift compiler are you suggesting we rebuild the compiler and then use it to run a Makefile for netcore? Thanks

@jeking3
Copy link

jeking3 commented Oct 11, 2017

That is not the official apache/thrift docker hub repository. It looks like it is maintained by someone else. The docker files that build it do not obtain files directly from the official github repo, but from a redistribution site or some kind.

@isaachier
Copy link
Contributor

Ya I don't know how that thrift old version (0.9.2) works for Go since there are breaking changes in the client library. I tried using a local checkout of 0.9.2 and got errors. @gideonkorir thanks for link to image.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thrift 0.9.2 logs a warning, but the build succeeds, so I think this is ok to add

[WARNING:/data/thrift/agent.thrift:20] No generator named 'netcore' could be found!

@yurishkuro yurishkuro merged commit 84a8310 into jaegertracing:master Oct 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants