Skip to content

Adding testable example documention#317

Merged
peterbourgon merged 1 commit intogo-kit:masterfrom
xmattstrongx:master
Jul 20, 2016
Merged

Adding testable example documention#317
peterbourgon merged 1 commit intogo-kit:masterfrom
xmattstrongx:master

Conversation

@xmattstrongx
Copy link
Copy Markdown
Contributor

Adding generic example as documentation for how to use this package.

Comment thread sd/etcd/example_test.go Outdated
{
logger = log.NewLogfmtLogger(os.Stdout)
logger = log.NewContext(logger).With("ts", log.DefaultTimestampUTC, "caller", log.DefaultCaller)
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The purpose of a test code is to show users how to wire things up. We don't need to be logging anything. Let's drop this, and all the debug logging below.

@xmattstrongx xmattstrongx force-pushed the master branch 2 times, most recently from c60268a to e316cd1 Compare July 11, 2016 15:30
@xmattstrongx
Copy link
Copy Markdown
Contributor Author

Any other thoughts/comments on this?

I am thinking/wondering if there might be value in adding some subscriber examples to this before merging.

@peterbourgon
Copy link
Copy Markdown
Member

peterbourgon commented Jul 15, 2016

So the idea with this example is to demonstrate how to use sd/etcd.Registrar, but as you note it's meant to be used together with sd/etcd.Subscriber. So, let's say that we have foosvc and barsvc, and barsvc depends on foosvc. Then barsvc will use a Subscriber on a key prefix like "/services/foosvc", and foosvc will register itself with keys like "/services/foosvc/1.2.3.4:8080".

This informs the example. The Registrar should include that prefix, something like

var (
    prefix   = "/services/foosvc/"  // known at compile time
    instance = "1.2.3.4:8080"       // taken from runtime or platform, somehow
    key      = prefix + instance
    value    = "http://" + instance // based on our transport
)

registrar := NewRegistrar(client, Service{
    Key:   key,
    Value: value,
}, log.NewNopLogger())

registrar.Register()

That's the write side. As you've done, I agree it's a good idea to use the client directly to demonstrate how things look in etcd,

entries, err := client.GetEntries(key)
fmt.Printf("%q (%v)\n", strings.Join(entries, ", "), err)

But we also have the opportunity to provide an end-to-end demonstration by constructing a Subscriber and seeing how many instances we get.

subscriber := NewSubscriber(client, prefix, someFactory, log.NewNopLogger())
fmt.Println(len(subscriber.Endpoints())) // hopefully 1

And now we can deregister and perform everything again, in reverse.

registrar.Deregister()

fmt.Println(len(subscriber.Endpoints()) // hopefully 0 now

entries, err = client.GetEntries(key)
fmt.Printf("%q (%v)\n", strings.Join(entries, ", "), err)

I think registering two distinct IPs is a bit of a distraction, and maybe a little confusing, because no single service would ever do that.

Does this make sense? What do you think?

@xmattstrongx
Copy link
Copy Markdown
Contributor Author

I do agree with your observations. I think adding the subscriber example provides definite value to the code base and Ill be implementing that soon/

However when registering two IPs I was thinking of more of a distributed system where multiple instances of a single service existed. This could all be masked as a single IP by a vip or load balancer though so perhaps the multi IP style example could easily be misconstrued.

@xmattstrongx
Copy link
Copy Markdown
Contributor Author

Strangely I am not getting the correct zero output when checkout len(subscriber.Endpoints) after Deregistering my instance (from code below).

registrar.Deregister()
fmt.Println(len(subscriber.Endpoints()) // hopefully 0 now

Although when I Deregister I am correctly seeing no value for given key when called GetEntries. I am thinking my fake factory implementation is incorrect.

@xmattstrongx
Copy link
Copy Markdown
Contributor Author

Well the code is complete as per the PR comments above. I am still not quite happy with it as I was unable to verify the subscriber functionality with my own client separate from this tests.

I do believe the example code to be accurate and my issues to be stemmed from my own errors when trying to get the factory in NewSubscriber implemented correctly.

@peterbourgon peterbourgon merged commit 68eecb0 into go-kit:master Jul 20, 2016
@peterbourgon
Copy link
Copy Markdown
Member

I can reproduce your error on an actual etcd server, which leads me to believe the DeleteOptions are somehow incorrect. I'm working on this in a separate branch.

@peterbourgon
Copy link
Copy Markdown
Member

Nope, it's a bug in Subscriber GetEntries. Fixing.

@xmattstrongx
Copy link
Copy Markdown
Contributor Author

Ok cool. Ill be on the lookout for the PR. Glad this exercise was able to sniff out a bug.

@peterbourgon
Copy link
Copy Markdown
Member

Ha, it was a weird race condition. FML. PR incoming.

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.

2 participants