Code cleanup #376

woodsaj · 2016-11-08T05:58:19Z

These are change pulled from the cluster PR.

Dieterbe · 2016-11-08T08:40:07Z

metrictank.go

-		log.Info("Shutting down %s consumer", w.key)
-		w.plugin.Stop()
+
+	// shutdown our reciever plugins.  These may take a while as we allow them


receive receive receive receive :) (edit: does not apply anymore if we switch to input)

Dieterbe · 2016-11-08T08:50:47Z

in/carbon/carbon.go

 		go c.handle(conn)
 	}
 }

+func (c *Carbon) Stop() {
+	c.listener.Close()


I think there's 2 issues here:

c.listener.AcceptTCP() will return an error when we close the listener, we log it as an error, but it's actually normal. unfortunately there's no elegant way to check which kind of error was returned by AcceptTCP, but there's some tricks you can do. see http://zhen.org/blog/graceful-shutdown-of-go-net-dot-listeners/ (I found out about this when reading https://github.com/Shopify/toxiproxy/blob/2526adc590a5f1e80d37dce7e39a68045eee250c/proxy.go#L137 the other day)

we don't seem to have anything that stops the c.handle goroutines, I think they will keep working as long as their client connection is established (possibly long after we closed the listener)

Dieterbe · 2016-11-08T08:54:09Z

in/kafkamdam/kafkamdam.go

@@ -27,6 +27,10 @@ type KafkaMdam struct {
 	StopChan chan int


seems like these two lines can be removed

Dieterbe · 2016-11-08T08:54:57Z

in/kafkamdm/kafkamdm.go

@@ -203,14 +207,10 @@ func (k *KafkaMdm) consumePartition(topic string, partition int32, partitionOffs
 }

 // Stop will initiate a graceful stop of the Consumer (permanent)
-//
-// NOTE: receive on StopChan to block until this process completes
+// and block until it stopped.


also here, we should probably remove StopChan attribute

Dieterbe · 2016-11-08T09:00:26Z

metrictank.go

-	clKafka "github.com/raintank/metrictank/mdata/clkafka"
-	clNSQ "github.com/raintank/metrictank/mdata/clnsq"
+	"github.com/raintank/metrictank/mdata/notifierKafka"
+	notifierNsq "github.com/raintank/metrictank/mdata/notifierNsq"


why do we need to override the import name for notifierNsq but not for notifierKafka?

Dieterbe · 2016-11-08T09:10:04Z

metrictank.go

-		sarama.Logger = l.New(os.Stdout, "[Sarama] ", l.LstdFlags)
-		inKafkaMdamInst.Start(metrics, metricIndex, usg)
+	/***********************************
+		Start our receivers


we have a naming problem:

in the docs and configs, we talk about inputs

in the code, we use in

in the main scope, we call them receivers

We should make this consistent to 1 terminology.
in is a bit too short to be a meaningful term in docs etc, but input is shorter than receiver, so I think we should call them input in all 3 places.

ok, lets standardize on "inputs"

Dieterbe · 2016-11-08T09:28:03Z

mdata/notifierKafka/notifierKafka.go

-	client, err := sarama.NewClient(cfg.Brokers, cfg.Config)
+func NewNotifierKafka(instance string, metrics mdata.Metrics, stats met.Backend) *NotifierKafka {
+	messagesPublished = stats.NewCount("cluster.messages-published")
+	messagesSize = stats.NewMeter("cluster.message_size", 0)


now the notifier plugins have their own copies of the metrics, but with the same keys. because it ultimately gets sent to statsd (for now) this doesn't really matter but I suspect this is not what was intended. we should probably add the notifier type into the metrics instead. (e.g. notifier.kafka.messages-published)

Dieterbe · 2016-11-08T09:31:49Z

mdata/notifierNsq/cfg.go

+	ProducerOpts      string
+	ConsumerOpts      string
+	PCfg              *nsq.Config
+	CCfg              *nsq.Config


with the exception of Enabled, all these variables don't need to exported anymore because we only use them in the package itself. in fact we probably also don't need the two separate files anymore. everything that's in cfg.go can be moved into notifierNsq.go (although you can also keep it separate if you want, i don't care much)

Dieterbe · 2016-11-08T09:32:39Z

mdata/notifierKafka/cfg.go

@@ -20,6 +21,9 @@ var Config *sarama.Config
 var OffsetDuration time.Duration
 var OffsetCommitInterval time.Duration


with the exception of Enabled, all these variables don't need to exported anymore because we only use them in the package itself. in fact we probably also don't need the two separate files anymore. everything that's in cfg.go can be moved into notifierKafka.go (although you can also keep it separate if you want, i don't care much)

in.carbon, in.kafkamdm, in.kafkamdam all kind of implemetned the in.Plugin interface, but didnt. This commit cleans things up so that all of the plugins work in the same way.

- rename CL* to Notifier* so it is clear what the structs are used for. - refactor each implimentation to be an isolated plugin that implements the mdata.NotifierHandler interface

We had 3 different handle functions built into the abstract In package, one for each type of handler. Given that the handle functions are specific to only 1 of the handler implimentations, it makes sense to just move them into the handlers directly. Also added some additional unit tests.

woodsaj · 2016-11-09T05:29:22Z

@Dieterbe can this be merged now?

Dieterbe

just a few more changes needed. please squash them in to the appropriate commits instead of making new commits, so we have a cleaner history (using rebase -i), thanks.

Dieterbe · 2016-11-09T06:33:04Z

mdata/notifierKafka/notifierKafka.go

-	messagesPublished = stats.NewCount("cluster.messages-published")
-	messagesSize = stats.NewMeter("cluster.message_size", 0)
+	messagesPublished = stats.NewCount("notifier.kafka.messages-published")
+	messagesSize = stats.NewMeter("notifier.kafka.message_size", 0)


dashboard.json has to be updated accordingly

Dieterbe · 2016-11-09T06:35:57Z

input/carbon/carbon.go

+				return
+			default:
+			}
+			log.Error(4, "Accept Error: %s", err.Error())


all log messages should have a prefix describing the module

Dieterbe · 2016-11-09T06:36:12Z

input/carbon/carbon.go

 			if io.EOF != err {
-				log.Error(4, err.Error())
+				log.Error(4, "Recv error: %s", err.Error())


all log messages should have a prefix describing the module

Dieterbe · 2016-11-09T06:43:07Z

input/carbon/carbon_test.go

+			t.Fatal(err)
+		}
+	}
+	time.Sleep(time.Millisecond * 100)


why sleep? (don't just tell me, comment would be better)

As soon as this goroutine ends, the socket is closed. If we dont sleep here the sockets will be closed before the packets have been processed by the kernel and passed to the application.

Dieterbe · 2016-11-09T06:44:25Z

input/carbon/carbon_test.go

+		Name:         "default",
+		RetentionStr: "1s:1d",
+	}
+	s.Pattern, err = regexp.Compile(".*")


can just use https://golang.org/pkg/regexp/#MustCompile here

woodsaj assigned Dieterbe Nov 8, 2016

Dieterbe suggested changes Nov 8, 2016

View reviewed changes

woodsaj added 8 commits November 8, 2016 23:37

make ingestion input plugins implement a defined interface

3f8858b

in.carbon, in.kafkamdm, in.kafkamdam all kind of implemetned the in.Plugin interface, but didnt. This commit cleans things up so that all of the plugins work in the same way.

cleanup metricPersist notifier plugins

0b5ec79

- rename CL* to Notifier* so it is clear what the structs are used for. - refactor each implimentation to be an isolated plugin that implements the mdata.NotifierHandler interface

remove unessecary pointer use

10d80de

standardise on "input" as the name of plugins for ingesting metrics

611e012

stopChan no longer needed.

71a0304

include the notifier type in metric names

6597795

dont export config variables

d42b979

woodsaj force-pushed the codeCleanup branch from 3cdb1c4 to 1a665a5 Compare November 8, 2016 15:41

Dieterbe suggested changes Nov 9, 2016

View reviewed changes

woodsaj force-pushed the codeCleanup branch from 13c99b5 to c8afdcf Compare November 9, 2016 07:19

support clean shutdowns of carbon listener. Add unit tests

22955ff

woodsaj force-pushed the codeCleanup branch from c8afdcf to 22955ff Compare November 9, 2016 07:26

Dieterbe added 2 commits November 9, 2016 09:33

update dashboard for new notifier metrics

a2c24b3

add missing log prefixes

9d866aa

Dieterbe approved these changes Nov 9, 2016

View reviewed changes

Dieterbe merged commit 7a55436 into master Nov 9, 2016

Dieterbe mentioned this pull request Nov 11, 2016

Notifier fixups #381

Merged

Dieterbe deleted the codeCleanup branch December 15, 2017 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code cleanup #376

Code cleanup #376

woodsaj commented Nov 8, 2016

Dieterbe Nov 8, 2016 •

edited

Loading

Dieterbe Nov 8, 2016

Dieterbe Nov 8, 2016

Dieterbe Nov 8, 2016

Dieterbe Nov 8, 2016

woodsaj Nov 8, 2016

Dieterbe Nov 8, 2016

woodsaj Nov 8, 2016

Dieterbe Nov 8, 2016

Dieterbe Nov 8, 2016

Dieterbe Nov 8, 2016

woodsaj commented Nov 9, 2016

Dieterbe left a comment

Dieterbe Nov 9, 2016

Dieterbe Nov 9, 2016

Dieterbe Nov 9, 2016

Dieterbe Nov 9, 2016

woodsaj Nov 9, 2016

Dieterbe Nov 9, 2016

		@@ -20,6 +21,9 @@ var Config *sarama.Config
		var OffsetDuration time.Duration
		var OffsetCommitInterval time.Duration

Code cleanup #376

Code cleanup #376

Conversation

woodsaj commented Nov 8, 2016

Dieterbe Nov 8, 2016 • 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

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

woodsaj commented Nov 9, 2016

Dieterbe 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

Dieterbe Nov 8, 2016 •

edited

Loading