*: multi-topic nsq_tail; automated docker builds; etc #957

soar · 2017-10-30T19:00:06Z

I've used NSQ for one of our projects - It is cool system, thanks!

But I had very annoying problem - We have more than 5 queues and for each I should start new container of nsq_tail:

version: '3.2'
services:
  nsqtail:
    image: nsqio/nsq
    command: /nsq_tail --topic=mytopic1 --lookupd-http-address=nsqlookupd:4161
  nsqtail2:
    image: nsqio/nsq
    command: /nsq_tail --topic=mytopic2 --lookupd-http-address=nsqlookupd:4161
  nsqtail3:
    image: nsqio/nsq
    command: /nsq_tail --topic=mytopic3 --lookupd-http-address=nsqlookupd:4161
...

I know that Docker is "lightweight" system... But not lightweight-enough for running too many containers for simple tasks. So my idea was to make nsq_tail listen for multiple topics at once and write all messages from them. Now I can do simply:

version: '3.2'
services:
  nsqtail:
    image: soarname/nsq
    command: /nsq_tail --topic=topic1 --topic=topic2 --topic=topic3 --lookupd-http-address=nsqlookupd:4161

And I see in logs:

nsqtail_1     | topic1 | test-message-1
nsqtail_1     | topic2 | test-message-2
nsqtail_1     | topic3 | test-message-3

Also I've fixed:

Dockerfile - now builds for Docker ecosystem are fully automated - you can check it here: soarname/nsq
.travis.yml - import path for building forks (without this line - I've got an error "use of internal package not allowed")
coverage.sh - fix path for goveralls
dist.sh - fix calling mktemp without XXX in template

mreiferson

Thanks for proposing these changes 😍 ! Just a few questions and comments.

mreiferson · 2017-10-31T15:06:19Z

.travis.yml

@@ -7,6 +7,7 @@ env:
  - GOARCH=amd64
  - GOARCH=386
 sudo: false
+go_import_path: github.com/nsqio/nsq


Can you explain why this is needed?

@mreiferson without this line - fork build fails, you can see example run here:

https://travis-ci.org/soar/nsq/jobs/294934597

package github.com/soar/nsq/apps/nsq_pubsub imports github.com/nsqio/nsq/internal/app: use of internal package not allowed

And here is Travis documentation for this line:

https://docs.travis-ci.com/user/languages/go/#Go-Import-Path

With this line any fork will work.

mreiferson · 2017-10-31T15:07:32Z

Dockerfile

@@ -1,10 +1,28 @@
+FROM golang:latest AS build


How large is the resulting container image with this approach?

The special part is the separate FROM alpine:3.6 below - this first image is only used for building, then the result is copied from this image to the separate final image. This technique is newly possible in ~~docker~~ moby 17.05.0-ce

Yep, curious about the results.

@mreiferson This is multistage-build feature from Docker 17.05+. First image is used for build, second - for distribution.

mreiferson · 2017-10-31T15:07:53Z

Dockerfile

+ && chmod +x /bin/dep \
+ && /bin/dep ensure \
+ && ./test.sh \
+ && ./coverage.sh --coveralls \


I don't think we need to run code coverage as part of this build?

@mreiferson Why not? In my projects I always have testing/coveraging as part of build process. For example if one test fails - build will be interrupted and broken image will not be uploaded to Docker repository. I think this is good practice. Yes, it has some impact on build time, but it is not too drastical.

Runnings tests here can be useful because they can fail the build. But in this case the coverage results are just thrown out.

mreiferson · 2017-10-31T15:09:44Z

apps/nsq_tail/nsq_tail.go

+	if err != nil {
+		log.Fatalf("ERROR: failed to write to os.Stdout - %s", err)
+	}
+	_, err = os.Stdout.Write(m.Body)


A side effect of this change is that the output can no longer be (easily) piped into subsequent processes, a common operation when using nsq_tail.

I suppose one option is to just multiplex the output without prefixing the topic name?

mreiferson · 2017-10-31T15:10:53Z

dist.sh

@@ -30,7 +30,7 @@ echo "... running tests"

 for os in linux darwin freebsd windows; do
    echo "... building v$version for $os/$arch"
-    BUILD=$(mktemp -d -t nsq)
+    BUILD=$(mktemp -d -t nsq-XXXXX)


Does this still work on OSX?

We've hit this before, not sure what happened 😝
thread starts at #718 (comment)

Cool, let's update this to reflect @ploxiln's suggestion in #718 (comment) ?

ploxiln · 2017-10-31T18:02:16Z

This would be better as two separate pull requests IMHO

mreiferson · 2017-10-31T19:00:04Z

This would be better as two separate pull requests IMHO

Agreed, unless we can quickly come to a conclusion around nsq_tail.

ploxiln · 2017-10-31T19:07:02Z

apps/nsq_tail/nsq_tail.go

 	}

-	for {
+	for _, consumer := range consumers {
 		select {
 		case <-consumer.StopChan:


This is not really useful: it will only wait for the first consumer to stop.

I don't think any consumers will stop before we tell them to though, they will try to reconnect. So this case could just be removed (and the loop as well). This just needs to wait for the sigChan and then tell all consumers to stop (as you have below). After that, to be most correct, it should then wait for all consumers to have stopped.

@ploxiln I've simply rewritten existing logic. Are you sure, that we need to have only waiting for SigChan?

It wasn't 100% correct before, but in the process of rewriting it you made it more nonsensical. The previous bare for loop is different in nature from a loop over all consumers.

nsqio#957 (comment)

ploxiln · 2017-11-03T21:51:09Z

Dockerfile

+
+WORKDIR /go/src/github.com/nsqio/nsq
+
+RUN go get github.com/mattn/goveralls \


goveralls isn't needed here since ./coverage.sh is not run here

You're right, yeah...

ploxiln · 2017-11-03T21:51:55Z

I found another little thing, but the updates look good, thanks.

according to nsqio#957 (review)

ploxiln · 2017-11-05T18:26:22Z

This looks good to me. I didn't want to be a bother about squashing commits, so I went ahead and did that in #960

I'll still wait a bit for @mreiferson's approval

ploxiln · 2017-11-05T18:46:50Z

probably worth mentioning: I've tested the nsq_tail multi-topic functionality, but not the docker image build, I don't have a new-enough version of docker/moby set up at the moment

soar · 2017-11-05T19:31:22Z

@ploxiln you don't need Docker to check that It builds successfully, here is automated build on Docker Hub: https://hub.docker.com/r/soarname/nsq/builds/

ploxiln · 2017-11-05T20:10:13Z

Ah, yes, I can pull and inspect that image. It comes out to 57.36 MB uncompressed, and seems to work fine.

with optional flag "--print-topic" to print topic name before message body

avoid "use of internal package not allowed" error in this case by forcing the import path to be as if it were the upstream repo

see nsqio#718 (comment)

now works for any $GOPATH, not just $HOME/gopath

build in a golang-based container, then copy resulting binaries into a minimal alpine-based container

ploxiln · 2017-11-05T22:41:59Z

(updated)

soar · 2017-11-06T13:37:15Z

@ploxiln @mreiferson Thank you!

mreiferson reviewed Oct 31, 2017

View reviewed changes

mreiferson changed the title ~~Better nsq_tail and automatic builds~~ *: multi-topic nsq_tail; automated docker builds; etc Oct 31, 2017

mreiferson added the chore label Oct 31, 2017

ploxiln reviewed Oct 31, 2017

View reviewed changes

soar added a commit to soar/nsq that referenced this pull request Nov 3, 2017

Remove coverage generation from Dockerfile, according to:

a439aab

nsqio#957 (comment)

soar added a commit to soar/nsq that referenced this pull request Nov 3, 2017

Remove unnneded loop over consumers, according to:

5bb43c7

nsqio#957 (comment)

ploxiln reviewed Nov 3, 2017

View reviewed changes

soar added a commit to soar/nsq that referenced this pull request Nov 5, 2017

goveralls isn't needed

2217b6c

according to nsqio#957 (review)

ploxiln mentioned this pull request Nov 5, 2017

nsq_tail multi-topic and build stuff #960

Closed

soar added 5 commits November 5, 2017 15:17

nsq_tail: multiple --topic support

abb3f49

with optional flag "--print-topic" to print topic name before message body

enable travis-ci to test other forks

845a786

avoid "use of internal package not allowed" error in this case by forcing the import path to be as if it were the upstream repo

dist.sh: cross-platform mktemp usage

0c8199b

see nsqio#718 (comment)

coverage.sh: more robust goveralls path

c521a35

now works for any $GOPATH, not just $HOME/gopath

docker image: multi-stage build

4480364

build in a golang-based container, then copy resulting binaries into a minimal alpine-based container

ploxiln force-pushed the feature/multitail branch from 2217b6c to 4480364 Compare November 5, 2017 20:21

mreiferson merged commit 9d6dad6 into nsqio:master Nov 6, 2017

jehiah mentioned this pull request Jan 29, 2018

nsq_tail: mv stdout logging to stderr #991

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: multi-topic nsq_tail; automated docker builds; etc #957

*: multi-topic nsq_tail; automated docker builds; etc #957

soar commented Oct 30, 2017

mreiferson left a comment

mreiferson Oct 31, 2017

soar Nov 1, 2017

mreiferson Oct 31, 2017

ploxiln Oct 31, 2017

mreiferson Oct 31, 2017

soar Nov 1, 2017

mreiferson Oct 31, 2017

soar Nov 1, 2017

ploxiln Nov 2, 2017

mreiferson Oct 31, 2017

mreiferson Oct 31, 2017

ploxiln Oct 31, 2017

mreiferson Oct 31, 2017

ploxiln commented Oct 31, 2017

mreiferson commented Oct 31, 2017

ploxiln Oct 31, 2017

soar Nov 1, 2017 •

edited

Loading

ploxiln Nov 2, 2017

ploxiln Nov 3, 2017

soar Nov 5, 2017

ploxiln commented Nov 3, 2017

ploxiln commented Nov 5, 2017

ploxiln commented Nov 5, 2017

soar commented Nov 5, 2017

ploxiln commented Nov 5, 2017

ploxiln commented Nov 5, 2017

soar commented Nov 6, 2017


		WORKDIR /go/src/github.com/nsqio/nsq

		RUN go get github.com/mattn/goveralls \

*: multi-topic nsq_tail; automated docker builds; etc #957

*: multi-topic nsq_tail; automated docker builds; etc #957

Conversation

soar commented Oct 30, 2017

mreiferson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ploxiln commented Oct 31, 2017

mreiferson commented Oct 31, 2017

Choose a reason for hiding this comment

soar Nov 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ploxiln commented Nov 3, 2017

ploxiln commented Nov 5, 2017

ploxiln commented Nov 5, 2017

soar commented Nov 5, 2017

ploxiln commented Nov 5, 2017

ploxiln commented Nov 5, 2017

soar commented Nov 6, 2017

soar Nov 1, 2017 •

edited

Loading