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

zookeeper registry #24

Merged
merged 6 commits into from May 16, 2016

Conversation

Projects
None yet
2 participants
@HeavyHorst
Contributor

HeavyHorst commented May 9, 2016

#17 I've started working on a zookeeper registry.

Everything seems to work, but some more testing may be necessary.

The watcher code is also a little bit complicated because zookeeper doesn't handle recursive watch and treats node and directory watches differently.

@asim

This comment has been minimized.

Member

asim commented May 9, 2016

Firstly, thanks very much for the contribution. I believe the zookeeper client library provides a method for testing with a TestCluster https://godoc.org/github.com/samuel/go-zookeeper/zk#TestCluster. I'll check out your plugin locally and have a play myself to see how it all works.

I can understand the pains of the watcher. Every underlying KV store whether it be consul, etcd or zookeeper provide different methods of tracking changes. Just have to do the best possible to maintain the Micro side interface so it provides consistent behaviour to the user.

@asim

This comment has been minimized.

Member

asim commented May 15, 2016

Have you tried to use the watcher? I tried to use it and got the following error

panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0xdfc88]

goroutine 23 [running]:
panic(0x5c0f80, 0xc82000a0e0)
    /usr/local/go/src/runtime/panic.go:481 +0x3e6
github.com/samuel/go-zookeeper/zk.(*Conn).nextXid(0x0, 0xa824b0)
    /Users/asim/checkouts/src/github.com/samuel/go-zookeeper/zk/conn.go:671 +0x48
github.com/samuel/go-zookeeper/zk.(*Conn).queueRequest(0x0, 0xc, 0x49a8e0, 0xc8200d8780, 0x49a940, 0xc82006e960, 0x0, 0x20)
    /Users/asim/checkouts/src/github.com/samuel/go-zookeeper/zk/conn.go:686 +0x25
github.com/samuel/go-zookeeper/zk.(*Conn).request(0x0, 0xc80000000c, 0x49a8e0, 0xc8200d8780, 0x49a940, 0xc82006e960, 0x0, 0x0, 0x0, 0x0)
    /Users/asim/checkouts/src/github.com/samuel/go-zookeeper/zk/conn.go:698 +0x7a
github.com/samuel/go-zookeeper/zk.(*Conn).Children(0x0, 0x68b990, 0xf, 0x0, 0x0, 0x0, 0x11837, 0x0, 0x0)
    /Users/asim/checkouts/src/github.com/samuel/go-zookeeper/zk/conn.go:709 +0x10a
github.com/HeavyHorst/go-plugins/registry/zookeeper.getServices(0x0, 0xc8201060c0, 0x0, 0x0)
    /Users/asim/checkouts/src/github.com/HeavyHorst/go-plugins/registry/zookeeper/util.go:54 +0x5d
github.com/HeavyHorst/go-plugins/registry/zookeeper.(*zookeeperWatcher).watch(0xc820106090)
    /Users/asim/checkouts/src/github.com/HeavyHorst/go-plugins/registry/zookeeper/watcher.go:126 +0x83
created by github.com/HeavyHorst/go-plugins/registry/zookeeper.newZookeeperWatcher
    /Users/asim/checkouts/src/github.com/HeavyHorst/go-plugins/registry/zookeeper/watcher.go:40 +0x12d
exit status 2

Simple code to test

package main

import (
    "fmt"

    zk "github.com/HeavyHorst/go-plugins/registry/zookeeper"
)

func main() {
    r := zk.NewRegistry()

    w, err := r.Watch()
    if err != nil {
        fmt.Println(err)
        return
    }

    for {
        r, err := w.Next()
        if err != nil {
            fmt.Println(err)
            return
        }
        fmt.Printf("%+v\n", r)
    }
}
}
cAddrs = append(cAddrs, addr)
}

This comment has been minimized.

@asim

asim May 15, 2016

Member

If there's no nodes, we should specify the default like the other implementations

if len(cAddrs) == 0 {
  cAddrs = []string{"127.0.0.1:2181"}
}
@asim

This comment has been minimized.

Member

asim commented May 15, 2016

After fixing that. The watcher exits immediately

2016/05/15 11:19:09 Connected to 127.0.0.1:2181
2016/05/15 11:19:10 Authenticated: id=95899290438926336, timeout=5000
zk: node does not exist
@HeavyHorst

This comment has been minimized.

Contributor

HeavyHorst commented May 15, 2016

Can you test the watcher again ? Should work now.
Zookeeper doesn't allow watchers on non existent paths so we need to create the prefix path if its not already there.

@HeavyHorst

This comment has been minimized.

Contributor

HeavyHorst commented May 15, 2016

The watcher only seems to work if at least one service is already registered when it is started:

go run test.go                                                                                                                                                                                                :(
2016/05/15 14:59:19 Connected to 172.17.0.4:2181
2016/05/15 14:59:19 Authenticated: id=95899106014855186, timeout=5000
&{Action:delete Service:0xc8204500c0}
&{Action:create Service:0xc820450360}
&{Action:delete Service:0xc82005eae0}
&{Action:create Service:0xc820115020}
&{Action:delete Service:0xc8201152c0}

I will fix this tomorrow when i find some time.

@HeavyHorst

This comment has been minimized.

Contributor

HeavyHorst commented May 15, 2016

I think it should work now:

[zk: localhost:2181(CONNECTED) 19] rmr /micro-registry

go run test.go                                                                                                                                                                                                :(
2016/05/15 15:34:05 Connected to 172.17.0.4:2181
2016/05/15 15:34:05 Authenticated: id=95899106014855215, timeout=5000

> --> After creating and deleting a service

&{Action:create Service:0xc82042e6c0}
&{Action:delete Service:0xc8201163c0}


./service --registry zookeeper --registry_address 172.17.0.4:2181
2016/05/15 15:34:09 Listening on [::]:34525
2016/05/15 15:34:09 Broker Listening on [::]:38178
2016/05/15 15:34:09 Registering node: greeter-b431b8b6-1aa1-11e6-a3e1-5c260a047464
2016/05/15 15:34:09 Connected to 172.17.0.4:2181
2016/05/15 15:34:09 Authenticated: id=95899106014855217, timeout=5000
^C2016/05/15 15:34:10 Deregistering node: greeter-b431b8b6-1aa1-11e6-a3e1-5c260a047464
@asim

This comment has been minimized.

Member

asim commented May 15, 2016

Getting some strange behaviour running the greeter server micro/examples/greeter/server. If I run the service, it re-registers periodically which is fine but when I kill the service I get the error 2016/05/15 16:52:56 zk: version conflict on deregister.

@HeavyHorst

This comment has been minimized.

Contributor

HeavyHorst commented May 16, 2016

2016/05/15 16:52:56 zk: version conflict

-> fixed

@asim

This comment has been minimized.

Member

asim commented May 16, 2016

Looks good. There's one last thing that I want to highlight but I think can be left to do afterwards. I really want to merge this in now. Because services re-register it causes a discovery system to fire events which means watchers are overloaded with broadcasts that aren't necessarily new updates.

In the consul registry implementation, we avoid this, by maintaining a cache of hashes for services that have been registered and only re-register when the hash changes. Otherwise in the Consul case, we pass the healthcheck. Like I mention, this can be left as something to be done afterwards and you can address it if you like since you already have good knowledge of how the zookeeper implementation works.

The reference code for example sake https://github.com/micro/go-micro/blob/master/registry/consul_registry.go#L108

    // create hash of service; uint64
    h, err := hash.Hash(s, nil)
    if err != nil {
        return err
    }

    // use first node
    node := s.Nodes[0]

    // get existing hash
    c.Lock()
    v, ok := c.register[s.Name]
    c.Unlock()

    // if it's already registered and matches then just pass the check
    if ok && v == h {
        // if the err is nil we're all good, bail out
        // if not, we don't know what the state is, so full re-register
        if err := c.Client.Agent().PassTTL("service:"+node.Id, ""); err == nil {
            return nil
        }
    }

@asim asim merged commit 497c817 into micro:master May 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@asim asim referenced this pull request May 16, 2016

Closed

Zookeeper Registry #17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment