Round out config reload #523

tylertreat · 2017-06-22T22:09:34Z

Implement config reload support for remaining options:

@nats-io/core

coveralls · 2017-06-22T22:29:40Z

Changes Unknown when pulling a0f32a1 on logtime_reload into ** on master**.

ColinSullivan1

LGTM.

ColinSullivan1 · 2017-06-27T20:56:51Z

server/reload.go

+		clients = make([]*client, len(server.clients))
+		i       = 0
+	)
+	for _, client := range server.clients {


It may be useful to add a comment to the effect that map iteration is where the randomness is introduced - that feature of go may not be readily apparent to future contributors.

coveralls · 2017-06-27T21:14:48Z

Changes Unknown when pulling 84d00a0 on logtime_reload into ** on master**.

petemiron

I approve with one comment for discussion. I'm not sure we should actively evict clients if max conns is adjusted, until we refine strategies for eviction. Otherwise looks great.

petemiron · 2017-06-27T22:05:53Z

server/reload.go

+		i       = 0
+	)
+	// Map iteration is random, which allows us to close random connections.
+	for _, client := range server.clients {


Should we actively evict until we can offer strategies for eviction or some of the client hints we discussed previously (lame duck mode)?

Discussed offline. Consensus was to leave this in for now. If you're decreasing max conns, there's probably a reason and you probably want the connections to decrease. I think this aligns with users' expectations.

kozlovic

I am worried about changing the type of exported fields.

kozlovic · 2017-06-28T15:28:54Z

server/monitor.go

@@ -381,7 +381,7 @@ type Varz struct {
 	*Info
 	*Options
 	Port             int               `json:"port"`
-	MaxPayload       int               `json:"max_payload"`
+	MaxPayload       int64             `json:"max_payload"`


I would be very careful here: this may break user code that embeds/uses public API to process monitoring. I would leave this as int (see below)

kozlovic · 2017-06-28T15:29:06Z

server/opts.go

@@ -54,7 +54,7 @@ type Options struct {
 	HTTPSPort      int           `json:"https_port"`
 	AuthTimeout    float64       `json:"auth_timeout"`
 	MaxControlLine int           `json:"max_control_line"`
-	MaxPayload     int           `json:"max_payload"`
+	MaxPayload     int64         `json:"max_payload"`


Same as monitor, this is a breaking change.

kozlovic · 2017-06-28T15:29:20Z

server/opts.go

@@ -245,7 +245,7 @@ func ProcessConfigFile(configFile string) (*Options, error) {
 		case "max_control_line":
 			opts.MaxControlLine = int(v.(int64))
 		case "max_payload":
-			opts.MaxPayload = int(v.(int64))
+			opts.MaxPayload = v.(int64)


Would leave it as a int.

kozlovic · 2017-06-28T15:37:44Z

server/reload.go

+// below the limit if necessary.
+func (m *maxConnOption) Apply(server *Server) {
+	server.mu.Lock()
+	var (


It may be as fast and more Go like to do:

var clients []*client clients = append(clients, server.clients...)

This doesn't work since server.clients is a map, not a slice.

Oh right, forgot about that.

kozlovic · 2017-06-28T15:38:28Z

server/reload.go

+// setting.
+type maxPayloadOption struct {
+	noopOption
+	newValue int64


Keep it a int here too.

kozlovic · 2017-06-28T15:39:11Z

server/reload.go

+	server.info.MaxPayload = m.newValue
+	server.generateServerInfoJSON()
+	for _, client := range server.clients {
+		atomic.StoreInt64(&client.mpay, m.newValue)


Only here, cast it to int64, as in: atomic.StoreInt64(&client.mpay, int64(m.newValue))

kozlovic · 2017-06-28T15:39:47Z

server/reload.go

+		case "maxcontrolline":
+			diffOpts = append(diffOpts, &maxControlLineOption{newValue: newValue.(int)})
+		case "maxpayload":
+			diffOpts = append(diffOpts, &maxPayloadOption{newValue: newValue.(int64)})


That would be a int

kozlovic · 2017-06-28T15:40:52Z

server/reload_test.go

+	}
+	if updated.WriteDeadline != 2*time.Second {
+		t.Fatalf("WriteDeadline is incorrect.\nexpected 2s\ngot: %s", updated.WriteDeadline)
+	}


A reason why we don't test MaxPayload here?

kozlovic · 2017-06-28T15:42:04Z

server/server.go

@@ -36,7 +36,7 @@ type Info struct {
 	SSLRequired       bool     `json:"ssl_required"` // DEPRECATED: ssl json used for older clients
 	TLSRequired       bool     `json:"tls_required"`
 	TLSVerify         bool     `json:"tls_verify"`
-	MaxPayload        int      `json:"max_payload"`
+	MaxPayload        int64    `json:"max_payload"`


Again, would not change that.

kozlovic · 2017-06-28T16:08:46Z

server/client.go

@@ -94,7 +94,7 @@ type client struct {
 	opts  clientOpts
 	start time.Time
 	nc    net.Conn
-	mpay  int
+	mpay  int64


Move this to top of structure (say after stats) because of this: golang/go#599

To see if this is still relevant, you could build the 32 bits (on a Linux VM) and try to run it and see if it crashes (that is, if you don't move the field)

coveralls · 2017-06-28T16:37:57Z

Changes Unknown when pulling 2a7af1b on logtime_reload into ** on master**.

tylertreat · 2017-06-28T16:40:25Z

Trying to track down what's causing these intermittent build failures...

kozlovic

Once you figure out what causes those failures (I assume they are independent of the changes in this PR)

kozlovic · 2017-06-28T16:09:49Z

server/client.go

@@ -94,7 +94,7 @@ type client struct {
 	opts  clientOpts
 	start time.Time
 	nc    net.Conn
-	mpay  int
+	mpay  int64


To see if this is still relevant, you could build the 32 bits (on a Linux VM) and try to run it and see if it crashes (that is, if you don't move the field)

…ailures.

wallyqs · 2017-06-28T17:20:17Z

server/reload.go

+// Apply is a no-op because the write deadline will be reloaded after options
+// are applied.
+func (w *writeDeadlineOption) Apply(server *Server) {
+	server.Noticef("Reloaded: write_deadline = %s", w.newValue)


Not sure if we'd want this, but if String() is implemented for the options being reloaded then could log once after calling Apply here? https://github.com/nats-io/gnatsd/pull/523/files#diff-76ed437f66106035c2352c7500aa8606R544

IMO logging in Apply gives more flexibility over what/how options are logged and also makes it harder to accidentally log something you shouldn't (like a password for example).

Tyler Treat added 7 commits June 22, 2017 12:59

Add support for reloading logtime and log_file

8f5aa04

Add support for reloading max_connections

11c2720

Add reload support for pid_file, max_control_line, and max_payload

b898b55

Replace mpay locking with atomic

d60085d

Support syslog config reload

5501e28

Add reload support for ping interval, max pings, and write deadline

88c864b

Fix grammatical mistake in docstring

a0f32a1

tylertreat requested review from ColinSullivan1 and petemiron June 27, 2017 17:45

tylertreat mentioned this pull request Jun 27, 2017

Implement config reload #524

Merged

6 tasks

ColinSullivan1 approved these changes Jun 27, 2017

View reviewed changes

Add comment about random map iteration

84d00a0

petemiron approved these changes Jun 27, 2017

View reviewed changes

kozlovic requested changes Jun 28, 2017

View reviewed changes

Address CR feedback

901a5c7

kozlovic reviewed Jun 28, 2017

View reviewed changes

Move mpay field for memory alignment

2a7af1b

kozlovic approved these changes Jun 28, 2017

View reviewed changes

Peter Miron added 2 commits June 28, 2017 12:46

added sudo: false to test .travis.yml failures.

cef37da

removed sudo: false. as sudo: true is required and causing separate f…

52e97e3

…ailures.

wallyqs reviewed Jun 28, 2017

View reviewed changes

tylertreat merged commit 446cf96 into master Jun 28, 2017

tylertreat deleted the logtime_reload branch June 28, 2017 19:35

ColinSullivan1 mentioned this pull request Jul 11, 2017

Hot configuration reload #338

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Round out config reload #523

Round out config reload #523

tylertreat commented Jun 22, 2017

coveralls commented Jun 22, 2017

ColinSullivan1 left a comment

ColinSullivan1 Jun 27, 2017

coveralls commented Jun 27, 2017

petemiron left a comment

petemiron Jun 27, 2017

tylertreat Jun 27, 2017

kozlovic left a comment

kozlovic Jun 28, 2017

kozlovic Jun 28, 2017

kozlovic Jun 28, 2017

kozlovic Jun 28, 2017

tylertreat Jun 28, 2017

kozlovic Jun 28, 2017

kozlovic Jun 28, 2017

kozlovic Jun 28, 2017

kozlovic Jun 28, 2017

kozlovic Jun 28, 2017

kozlovic Jun 28, 2017

kozlovic Jun 28, 2017

kozlovic Jun 28, 2017

coveralls commented Jun 28, 2017

tylertreat commented Jun 28, 2017

kozlovic left a comment

kozlovic Jun 28, 2017

wallyqs Jun 28, 2017

tylertreat Jun 28, 2017

Round out config reload #523

Round out config reload #523

Conversation

tylertreat commented Jun 22, 2017

coveralls commented Jun 22, 2017

ColinSullivan1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 27, 2017

petemiron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozlovic 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

coveralls commented Jun 28, 2017

tylertreat commented Jun 28, 2017

kozlovic 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