search

joshk / pusher

Pusher server in Go
37 stars 15 forks source link

Move host (formerly Endpoint) to client #6

Closed jnunemaker closed 11 years ago

jnunemaker commented 11 years ago

i have no idea what i am doing

But I think this would make the reading and writing of the host (formerly Endpoint) safe.

jnunemaker commented 11 years ago

/cc @alindeman

joshk commented 11 years ago

WHAT ARE YOU DOING PLAYING WITH GO?? :)

joshk commented 11 years ago

Do you think it is overkill to use a Lock? And are other libs using this sort of pattern?

jnunemaker commented 11 years ago

LOLOLOL. OH HAI JOSH.

I copied the idea from the ServerList in gomemcached: https://github.com/bradfitz/gomemcache/blob/master/memcache/selector.go#L37-L84 https://github.com/bradfitz/gomemcache/blob/master/memcache/memcache.go#L116-L117

It seems to me that one would only set the host and then never change it, which makes me think the lock is overkill. BUTTTTTT, if someone did change it, chaos would ensue. I can remove it. I do not feel passionate either way.

alindeman commented 11 years ago

It seems to me that one would only set the host and then never change it, which makes me think the lock is overkill. BUTTTTTT, if someone did change it, chaos would ensue. I can remove it. I do not feel passionate either way.

I support removing it and assuming it won't change as it's being used ... and compel folks do their own locking or serialization if they do.

To me, it seems like the 80% case (maybe even the 99% case?) is creating a client and never changing the host.

I might make it Host (capital) to point out that it's accessible.

jnunemaker commented 11 years ago

Lock removed. Changed host to Host.

jnunemaker commented 11 years ago

In addition, I would move to do the same thing to secure or even just change it to SetScheme and default to http or something, but I can do that in a separate pull.

jnunemaker commented 11 years ago

I have also updated the formerly outdated title of the pull request since the lock is gone.

timonv commented 11 years ago

Alindeman:

I support removing it and assuming it won't change as it's being used ... and compel folks do their own locking or serialization if they do.

+5. F me, more people should do this.

jnunemaker:

I have also updated the formerly outdated title of the pull request since the lock is gone.

<3

In addition, I would move to do the same thing to secure or even just change it to SetScheme and default to http or something, but I can do that in a separate pull.

This pull would be a great place for that. The party is here!

joshk commented 11 years ago

You :metal: my good sir!