make lookup functionality pluggable #86

ploxiln · 2014-10-06T17:41:01Z

We'd like to be able to override how a consumer queries from NSQLookupds.
I think this approach would also allow others to switch NSQLookupd for etcd, zookeeper,
etc.

This is still a WIP but I hope you can see where I'm going with this.
Suggestions and Criticism welcome.

cc @oliver-bitly @jehiah @mreiferson

oliver-exbitly · 2014-10-06T19:07:46Z

consumer.go

-	numLookupd := len(r.lookupdHTTPAddrs)
-	r.mtx.RUnlock()
-	if numLookupd != 0 && atomic.LoadInt32(&r.stopFlag) == 0 {
+	have_lookup_sources := r.lookupProvider.HasSources()


--> has_lookup_sources

ploxiln · 2014-10-06T19:29:48Z

It's been pointed out that this could leave a lot more work than is warranted for someone who wants to continue using nsqlookupd sources but wants a simple filter on the returned nsqds. I considered making the default lookup provider public, in an attempt to make it re-usable with embedding. But it's also been suggested that the consumer could optionally have a filter attached, instead of all this.

Thoughts?

mreiferson · 2014-10-06T22:05:54Z

consumer.go

-	r.lookupdHTTPAddrs = append(r.lookupdHTTPAddrs, addr)
-	numLookupd := len(r.lookupdHTTPAddrs)
-	r.mtx.Unlock()
+	was_connected := atomic.SwapInt32(&r.connectedFlag, 1)


STYLE POLICE 👮 this isn't python plo - wasConnected

mreiferson · 2014-10-07T01:15:56Z

@ploxiln in light of the fact that nsqlookupd is going away (eventually, see nsqio/nsq#379) I think it makes more sense to just expose a callback that would filter the list returned from "discovery" (whatever the current underlying implementation may be).

jehiah · 2014-10-07T01:51:18Z

I think this looks good as is. I think the interface is more idiomatic than the callback approach, but one could argue that's just a preference of implementation.

@mreiferson in case it wasn't obvious, this give the ability to mange swimlanes using go-nsq similar to how we do with pynsq.

If i were to brainstorm implementation options, another possible approach would be to give control of polling lookupd's to the LookupProvider such that it takes over what are now LookupdPollInterval config values. The interface then would essentially expose a channel that pushes updated endpoints that have been discovered. That would support more than just timer based discovery.

mreiferson · 2014-10-07T02:06:59Z

I wasn't suggesting an actual callback, an interface would be fine.

I'm saying that simplification might be useful if all you're missing is the ability to filter the set of returned nsqd.

type Discoverer interface {
   Filter([]string) []string
}

The default implementation would pass through unfiltered.

ploxiln · 2014-10-07T02:21:56Z

That would indeed be sufficient for our use case.

ploxiln · 2014-10-07T18:42:48Z

I've put up the simple filter function alternative. I also considered a version that used

type HostPort struct {
    Host string
    Port int
}

which might be more convenient for the filtering function, and doesn't add any parsing / string forming steps, just defers them to after the filter.

Further opinions?

mreiferson · 2014-10-07T19:49:30Z

net.TCPAddr is exactly that type, I like the idea of not having to parse the addresses in the callback.

ploxiln · 2014-10-07T19:52:10Z

right, I was going to ask where that was if it already existed

ploxiln · 2014-10-07T19:53:39Z

Well TCPAddr contains an IP, not a dns name

mreiferson · 2014-10-07T19:55:33Z

Ahh, bummer.

Guess it would need a custom type then, in which case I probably prefer passing around just the string.

ploxiln · 2014-10-07T23:06:17Z

do we think this is as good as we can agree on? until unspoken eventualities transform the lookupd problem space?

mreiferson · 2014-10-08T01:16:05Z

I think the only question is whether or not this should be exposed as an actual interface rather than callback.

It would afford us certain opportunities if we wanted to expose other functionality in the future (by type asserting to other function-exposing interfaces).

What to name it?

oliver-exbitly · 2014-10-09T16:09:25Z

DiscoveryFilter is fine. Alternatively you could name it NSQDAddrFilter.

ploxiln · 2014-10-09T19:46:33Z

You might see what @mreiferson means if you look way up at his original suggestion, an interface with a Filter() method. The idea is that you could add more methods later (by adding more comprehensive / different interfaces), and type check to see if a particular one has been implemented. It could be used for any sort of behavior tweaking callback I guess. The most annoying choice to make is what to call the consumer method to set the tweaks object - setConsumerTweaks()? do we really want such a thing? I can pretty quickly bang out what that would look like anyway...

ploxiln · 2014-10-09T21:02:44Z

Updated. The idea is we can, later, specify more interfaces that a "ConsumerBehavior" thingie can implement. Is this the kind of thing you had in mind @mreiferson ?

mreiferson · 2014-10-09T21:04:49Z

consumer.go

+// DiscoveryFilter interface
+// one of the interfaces an argument to setConsumerBehavior() can satisfy
+// for filtering the nsqds returned from discovery via nsqlookupd
+type DiscoveryFilter interface {


hmmm, thinking out loud: DiscoveryProxy, DiscoveryDelegate, DiscoveryHandler

actually, sorry I get where you're going with this...

mreiferson · 2014-10-09T21:06:26Z

Yes, this is what I had in mind, just gonna simmer on the naming for a bit... nice work

mreiferson · 2014-10-20T20:26:24Z

should we get this in?

ploxiln · 2014-10-20T20:37:15Z

Have your thoughts on naming condensed sufficiently? I'll go ahead and squash...

mreiferson · 2014-10-20T20:43:01Z

consumer.go

@@ -180,6 +189,10 @@ func (r *Consumer) SetLogger(l logger, lvl LogLevel) {
 	r.logLvl = lvl
 }

+func (r *Consumer) SetConsumerBehavior(cb interface{}) {


The alternative to exposing a method to set this would be to make it a field of the Config struct.

I don't know if that's better or worse, just wanted to talk it through.

no matter where we end up, I think this should be called BehaviorDelegate (I think the use of the word Consumer is redundant here) and I think it needs Delegate because a) we've used it elsewhere and b) you're not setting the behavior here you're setting the thing that modifies the behavior 😁

If we keep this a method, we could check that the "delegate" fulfills one of the interfaces, or even return a list or count of the interfaces it does match, for debugging purposes. Maybe we don't really want to do that, just an idea.

ploxiln · 2014-10-20T21:46:59Z

renamed. could still move the setting of it to the Config struct if desired

mreiferson · 2014-10-20T21:58:26Z

Yea, I like that idea (panic if it does not satisfy at least one of the valid interfaces)

mreiferson · 2014-10-20T21:58:49Z

That is one benefit of it being a function rather than a field on Config, so perhaps that settles it.

ploxiln · 2014-10-21T14:19:57Z

I pushed up the panic() addition.

I think it's less-good than returning, say, a count of matched interfaces, because you might intend to augment two behaviors, but only augment one and not be sure, and also you might want to clear a behavior delegate you set (though I guess that could be racy).

mreiferson · 2014-10-21T15:07:20Z

We've taken the approach that all of the configuration needs to happen up front, before connection, and is otherwise undefined.

It's a toss up whether these situations should panic when used incorrectly or just error.

I'm still slowly forming stronger opinions around which of these approaches I prefer, but for right now I think it would be consistent with the rest of the package if it panicked (we can decide to change panicking vs erroring later).

ploxiln · 2014-10-21T17:25:35Z

squashed

ploxiln · 2014-10-21T22:56:07Z

I pushed up another tweak. Are we all agreed? Shall I final squash?

oliver-exbitly · 2014-10-21T23:01:03Z

LGTM

mreiferson · 2014-10-21T23:25:34Z

I don't like the tone of your voice plo

mreiferson · 2014-10-21T23:25:44Z

but 👍

for modifying nsq consumer behavior at some defined points panic()s if the behavior delegate casts to no known interfaces currently just supports DiscoveryFilter (with a Filter() method), to filter the nsqd addresses resulting from discovery (via nsqlookupd)

ploxiln · 2014-10-21T23:31:39Z

jeez, I have to walk on eggshells around here these days

mreiferson · 2014-10-21T23:32:31Z

❤️

oliver-exbitly · 2014-10-21T23:35:13Z

Welcome to funemployment

make lookup functionality pluggable

ploxiln self-assigned this Oct 6, 2014

ploxiln added the enhancement label Oct 6, 2014

oliver-exbitly reviewed Oct 6, 2014
View reviewed changes

mreiferson reviewed Oct 6, 2014
View reviewed changes

mreiferson reviewed Oct 9, 2014
View reviewed changes

mreiferson reviewed Oct 20, 2014
View reviewed changes

ploxiln force-pushed the pluggable_lookup_provider_86 branch from 9d2e856 to 20632a4 Compare October 20, 2014 20:45

ploxiln force-pushed the pluggable_lookup_provider_86 branch 2 times, most recently from 8b286b2 to 6d83da1 Compare October 21, 2014 17:23

ploxiln force-pushed the pluggable_lookup_provider_86 branch from b811579 to 72faeb5 Compare October 21, 2014 23:29

mreiferson added a commit that referenced this pull request Oct 22, 2014

Merge pull request #86 from ploxiln/pluggable_lookup_provider_86

61012eb

make lookup functionality pluggable

mreiferson merged commit 61012eb into nsqio:master Oct 22, 2014

ploxiln removed their assignment Aug 31, 2015

ploxiln deleted the pluggable_lookup_provider_86 branch October 17, 2021 17:08

make lookup functionality pluggable #86

make lookup functionality pluggable #86

Conversation

ploxiln commented Oct 6, 2014

oliver-exbitly Oct 6, 2014

Choose a reason for hiding this comment

ploxiln commented Oct 6, 2014

mreiferson Oct 6, 2014

Choose a reason for hiding this comment

mreiferson commented Oct 7, 2014

jehiah commented Oct 7, 2014

mreiferson commented Oct 7, 2014

ploxiln commented Oct 7, 2014

ploxiln commented Oct 7, 2014

mreiferson commented Oct 7, 2014

ploxiln commented Oct 7, 2014

ploxiln commented Oct 7, 2014

mreiferson commented Oct 7, 2014

ploxiln commented Oct 7, 2014

mreiferson commented Oct 8, 2014

oliver-exbitly commented Oct 9, 2014

ploxiln commented Oct 9, 2014

ploxiln commented Oct 9, 2014

mreiferson Oct 9, 2014

Choose a reason for hiding this comment

mreiferson Oct 9, 2014

Choose a reason for hiding this comment

mreiferson commented Oct 9, 2014

mreiferson commented Oct 20, 2014

ploxiln commented Oct 20, 2014

mreiferson Oct 20, 2014

Choose a reason for hiding this comment

mreiferson Oct 20, 2014

Choose a reason for hiding this comment

ploxiln Oct 20, 2014

Choose a reason for hiding this comment

ploxiln commented Oct 20, 2014

mreiferson commented Oct 20, 2014

mreiferson commented Oct 20, 2014

ploxiln commented Oct 21, 2014

mreiferson commented Oct 21, 2014

ploxiln commented Oct 21, 2014

ploxiln commented Oct 21, 2014

oliver-exbitly commented Oct 21, 2014

mreiferson commented Oct 21, 2014

mreiferson commented Oct 21, 2014

ploxiln commented Oct 21, 2014

mreiferson commented Oct 21, 2014

oliver-exbitly commented Oct 21, 2014