nsqd/nsqlookupd: add --broadcast-address flag #142

dustismo · 2013-01-24T19:31:46Z

replaces pull request #141

adds a "broadcast_address" field to the producer objects in nsqlookupd, and the corresponding --broadcast-address option to nsqd.

Additional thoughts?

mreiferson · 2013-01-24T21:59:45Z

@dustismo thanks for taking a stab at this, I'll take a look in a few hours.

FYI, in the future feel free to just continue pushing changes to existing pull requests until we either merge or close the issue rather than opening new ones. (we squash intermediate commits into logical units before merging anyway...)

dustismo · 2013-01-24T22:09:57Z

@mreiferson Yeah, sorry about the new pull. I wanted to start from a fresh branch rather then undo all the previous changes. Will not do that in the future.

mreiferson · 2013-01-25T03:05:32Z

nsqd/lookup.go

@@ -28,6 +28,8 @@ func (n *NSQd) lookupLoop() {
 			ci["tcp_port"] = n.tcpAddr.Port
 			ci["http_port"] = n.httpAddr.Port
 			ci["address"] = hostname
+			ci["broadcast_address"] = n.options.broadcastAddress


so thinking through this a bit, I think what makes the most sense is for this to send both hostname and broadcast_address keys in the JSON dictionary.

on the nsqlookupd side we'll need to "auto-detect" based on the JSON coming in (old nsqd peers will be sending just address and new peers will be sending hostname and broadcast_address)

mreiferson · 2013-01-25T03:11:26Z

the only other addition that probably belongs in this pull request is to update nsqadmin to display both values (hostname and broadcast_address)... but I understand if you don't want to step in the hell that is Go templating :)

dustismo · 2013-01-25T22:39:24Z

think these last commits cover all the comments above. let me know of any additional thoughts.

mreiferson · 2013-01-25T23:08:23Z

@dustismo, nice work.

Last request... can you update tests in two spots to validate these field changes?

nsqlookupd/nsqlookupd_test.go in TestBasicLookupd()
nsq/cluster_test.go in TestNsqdToLookupd()

thanks

dustismo · 2013-01-25T23:50:19Z

@mreiferson sure thing, though I am a bit confused about how this cluster_test works.

I see this output

starting nsqlookupd
starting nsqd --data-path=/tmp --lookupd-tcp-address=127.0.0.1:4160

but I con't figure out where that is happening. can you point me to it?

mreiferson · 2013-01-26T00:06:46Z

test.sh in the root of the repo starts things (when necessary) and runs go test in the right sub-dirs.

The cluster test file depends on test.sh to start local instances. We structured it like this because the cluster test is a layer above any particular daemon and can't start an instance in-process.

mreiferson · 2013-01-30T21:57:44Z

@dustismo looks like a one of the recent merges conflicts with this... can you rebase whenever you decide to get back to this?

dustismo · 2013-02-04T23:35:28Z

@mreiferson This should cover it I think. Let me know if you see anything else. Also, sorry, I did a merge of master rather then a rebase, is that a problem?

mreiferson · 2013-02-05T00:31:15Z

nsqlookupd/http.go

@@ -176,7 +176,8 @@ func tombstoneTopicProducerHandler(w http.ResponseWriter, req *http.Request) {
 	log.Printf("DB: setting tombstone for producer@%s of topic(%s)", node, topicName)
 	producers := lookupd.DB.FindProducers("topic", topicName, "")
 	for _, p := range producers {
-		thisNode := fmt.Sprintf("%s:%d", p.Address, p.HttpPort)
+		thisNode := fmt.Sprintf("%s:%d", p.BroadcastAddress, p.HttpPort)
+		log.Println("THIS NODE: " + thisNode + " EQUALS: " + node)


we can drop this debug line

mreiferson · 2013-02-05T00:41:39Z

re: merge vs rebase... we take the approach that while we're working in branches we strictly rebase on master so as to not introduce merge commits.

when we're ready for this to go in we'll rebase/squash down to one commit and merge that into master.

dustismo · 2013-02-05T20:58:30Z

@mreiferson I think that covers the broadcast-address flag addition for nsqlookupd.

mreiferson · 2013-02-05T21:18:09Z

nsqlookupd/lookup_protocol_v1.go

@@ -231,7 +237,10 @@ func (p *LookupProtocolV1) IDENTIFY(client *ClientV1, reader *bufio.Reader, para
 	if err != nil {
 		log.Fatalf("ERROR: unable to get hostname %s", err.Error())
 	}
-	data["address"] = hostname
+	data["address"] = lookupd.broadcastAddress //TODO: remove for 1.0


address should continue to be just hostname (despite the fact that broadcastAddress defaults to hostname) for backward compatibility

mreiferson · 2013-02-05T21:31:43Z

the nsqlookupd side changes look good... nsqd can now try to use BroadcastAddress if non-empty and Address otherwise at https://github.com/dustismo/nsq/blob/broadcast_address_flag/nsqd/lookup.go#L147.

Related to this, in https://github.com/dustismo/nsq/blob/broadcast_address_flag/nsq/lookup_peer.go#L23, we need to add BroadcastAddress string json:"broadcast_address"`` so that it will automagically decode into the structure.

dustismo · 2013-02-05T22:04:31Z

@mreiferson hows that?

mreiferson · 2013-02-06T02:27:51Z

sweet, i think this looks good!

can you do a final rebase on bitly/master and squash down to one commit?

add broadcast address update test with broadcast addr format format add hostname key to lookup add hostname field fmt update test add broadcast address and hostname to nodes admin page fmt more Address changes update to use broadcast address with identify and all throughout the lookup update test for broadcast_address test broadcast address remove debug line add broadcast flag to lookupd fmt update readme switch address back to hostname for backward compat update to use broadcast address fmt

dustismo · 2013-02-06T18:00:25Z

phew, rebase and squash was a new git adventure for me :). Think it all looks good now.

mreiferson · 2013-02-06T18:37:01Z

thanks for the contribution, LGTM

nsqd/nsqlookupd: add --broadcast-address flag

…ation - responsibility has been moved to Consumer startStopContinueBackoff - see PR nsqio#147 and nsqio#142

mreiferson mentioned this pull request Jan 24, 2013

add hostname flag to nsqd #141

Closed

mreiferson reviewed Jan 25, 2013
View reviewed changes

mreiferson reviewed Feb 5, 2013
View reviewed changes

mreiferson added a commit that referenced this pull request Feb 6, 2013

Merge pull request #142 from dustismo/broadcast_address_flag

6b0285d

nsqd/nsqlookupd: add --broadcast-address flag

mreiferson merged commit 6b0285d into nsqio:master Feb 6, 2013

mreiferson mentioned this pull request Jan 13, 2014

remove deprecated 'address' field #290

Merged

arussellsaw pushed a commit to arussellsaw/nsq that referenced this pull request Mar 5, 2018

update FullJitterStrategy Calculate to not bound by cfg.MaxBackoffDur…

9d87c3c

…ation - responsibility has been moved to Consumer startStopContinueBackoff - see PR nsqio#147 and nsqio#142

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nsqd/nsqlookupd: add --broadcast-address flag #142

nsqd/nsqlookupd: add --broadcast-address flag #142

dustismo commented Jan 24, 2013

mreiferson commented Jan 24, 2013

dustismo commented Jan 24, 2013

mreiferson Jan 25, 2013

mreiferson commented Jan 25, 2013

dustismo commented Jan 25, 2013

mreiferson commented Jan 25, 2013

dustismo commented Jan 25, 2013

mreiferson commented Jan 26, 2013

mreiferson commented Jan 30, 2013

dustismo commented Feb 4, 2013

mreiferson Feb 5, 2013

mreiferson commented Feb 5, 2013

dustismo commented Feb 5, 2013

mreiferson Feb 5, 2013

mreiferson commented Feb 5, 2013

dustismo commented Feb 5, 2013

mreiferson commented Feb 6, 2013

dustismo commented Feb 6, 2013

mreiferson commented Feb 6, 2013

nsqd/nsqlookupd: add --broadcast-address flag #142

nsqd/nsqlookupd: add --broadcast-address flag #142

Conversation

dustismo commented Jan 24, 2013

mreiferson commented Jan 24, 2013

dustismo commented Jan 24, 2013

mreiferson Jan 25, 2013

Choose a reason for hiding this comment

mreiferson commented Jan 25, 2013

dustismo commented Jan 25, 2013

mreiferson commented Jan 25, 2013

dustismo commented Jan 25, 2013

mreiferson commented Jan 26, 2013

mreiferson commented Jan 30, 2013

dustismo commented Feb 4, 2013

mreiferson Feb 5, 2013

Choose a reason for hiding this comment

mreiferson commented Feb 5, 2013

dustismo commented Feb 5, 2013

mreiferson Feb 5, 2013

Choose a reason for hiding this comment

mreiferson commented Feb 5, 2013

dustismo commented Feb 5, 2013

mreiferson commented Feb 6, 2013

dustismo commented Feb 6, 2013

mreiferson commented Feb 6, 2013