Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Default peer service name to blank as it is required in Thrift Endpoint #47

Merged
merged 3 commits into from
Sep 9, 2016

Conversation

yurishkuro
Copy link
Member

Fixes #45

@@ -72,7 +72,7 @@ public Tracer getTracer() {
public Endpoint getPeer() {
synchronized (this) {
if (peer == null) {
peer = new Endpoint();
peer = new Endpoint(0, (short) 0, "");
Copy link
Contributor

Choose a reason for hiding this comment

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

might want to return null.. ex null endpoint is ok, it is null serviceName that's the problem. an empty endpoint w/o an ip or serviceName or port, is probably worse than null.

@codefromthecrypt
Copy link
Contributor

PS verified this fixes it!

@badiib
Copy link

badiib commented Sep 9, 2016

LGTM pending the null conversation.

@yurishkuro yurishkuro changed the title Default peer service name to blank as it is require in Thrift Endpoint Default peer service name to blank as it is required in Thrift Endpoint Sep 9, 2016
@@ -21,15 +21,15 @@
*/
package com.uber.jaeger;

import com.twitter.zipkin.thriftjava.Endpoint;
import com.uber.jaeger.utils.Utils;
import io.opentracing.tag.Tags;
Copy link
Member Author

Choose a reason for hiding this comment

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

intelliJ kept insisting, so I let it

@yurishkuro
Copy link
Member Author

OK I figured it out. It was really a bug: reusing getPeer method as both lazy initializer and getter. I split them into plain getter and getOrMakePeer, the latter is only used by the methods that actually set the values on the peer, so the defaulting makes a lot more sense.

@yurishkuro yurishkuro merged commit c3e85be into master Sep 9, 2016
@yurishkuro
Copy link
Member Author

released as 0.8.1

@codefromthecrypt
Copy link
Contributor

thanks

On Sat, Sep 10, 2016 at 4:29 AM, Yuri Shkuro notifications@github.com
wrote:

released as 0.8.1


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#47 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAD61ytUxesvrWAeQS9o8tDqn6kRWdB4ks5qocG1gaJpZM4J4nqH
.

@yurishkuro yurishkuro deleted the do-not-require-peer-service-name branch December 23, 2017 21:56
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.

3 participants