Merge pull request #747 from travisturner/730-internal-port

move InternalPort config option out from under `cluster`

config.go

@@ -14,9 +14,7 @@
 
 package pilosa
 
-import (
-	"time"
-)
+import "time"
 
 // Cluster types.
 const (
@@ -51,8 +49,9 @@ var ClusterTypes = []string{ClusterNone, ClusterStatic, ClusterHTTP, ClusterGoss
 
 // Config represents the configuration for the command.
 type Config struct {
-	DataDir string `toml:"data-dir"`
-	Bind    string `toml:"bind"`
+	DataDir      string `toml:"data-dir"`
+	Bind         string `toml:"bind"`
+	InternalPort string `toml:"internal-port"`
 
 	Cluster struct {
 		ReplicaN      int      `toml:"replicas"`
@@ -60,7 +59,6 @@ type Config struct {
 		Hosts         []string `toml:"hosts"`
 		InternalHosts []string `toml:"internal-hosts"`
 		PollInterval  Duration `toml:"poll-interval"`
-		InternalPort  string   `toml:"internal-port"`
 		GossipSeed    string   `toml:"gossip-seed"`
 		LongQueryTime Duration `toml:"long-query-time"`
 	} `toml:"cluster"`
@@ -114,19 +112,24 @@ func (c *Config) Validate() error {
 		if c.Cluster.ReplicaN > len(c.Cluster.Hosts) {
 			return ErrConfigReplicaNInvalid
 		}
-		if len(c.Cluster.Hosts) != len(c.Cluster.InternalHosts) {
-			return ErrConfigHostsMismatch
-		}
 		if !foundItem(c.Cluster.Hosts, c.Bind) {
 			return ErrConfigHostsMissing
 		}
-		if !ContainsSubstring(c.Cluster.InternalPort, c.Cluster.InternalHosts) {
+	}
+	if c.Cluster.Type == ClusterHTTP {
+		if len(c.Cluster.Hosts) != len(c.Cluster.InternalHosts) {
+			return ErrConfigHostsMismatch
+		}
+		// TODO: this seems like an odd check; it's just ensuring that InternalPort
+		// matches any one substring from any of the InternalHosts.
+		// I suggest we either remove this completely or make it actually check
+		// the port portion of the address for this node. (note that this only applies
+		// to the http broadcaster, so if we simply use gossip for all implementations
+		// we can remove this).
+		if !ContainsSubstring(c.InternalPort, c.Cluster.InternalHosts) {
 			return ErrConfigBroadcastPort
 		}
 	}
-	if c.Cluster.Type == ClusterGossip && !StringInSlice(c.Cluster.GossipSeed, c.Cluster.InternalHosts) {
-		return ErrConfigGossipSeed
-	}
 
 	return nil
 }

config_test.go

@@ -26,7 +26,7 @@ func Test_NewConfig(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	c.Cluster.InternalPort = pilosa.DefaultInternalPort
+	c.InternalPort = pilosa.DefaultInternalPort
 	c.Cluster.InternalHosts = []string{"localhost:14004", "localhost:14001"}
 	if err := c.Validate(); err != pilosa.ErrConfigBroadcastPort {
 		t.Fatal(err)
@@ -47,11 +47,6 @@ func Test_NewConfig(t *testing.T) {
 
 	c.Cluster.ReplicaN = 2
 	c.Cluster.Type = pilosa.ClusterGossip
-	c.Cluster.GossipSeed = "localhost:10101"
-	if err := c.Validate(); err != pilosa.ErrConfigGossipSeed {
-		t.Fatal(err)
-	}
-
 	c.Cluster.GossipSeed = "localhost:14000"
 	if err := c.Validate(); err != nil {
 		t.Fatal(err)

ctl/server.go

@@ -26,6 +26,7 @@ func BuildServerFlags(cmd *cobra.Command, srv *server.Command) {
 	flags := cmd.Flags()
 	flags.StringVarP(&srv.Config.DataDir, "data-dir", "d", "~/.pilosa", "Directory to store pilosa data files.")
 	flags.StringVarP(&srv.Config.Bind, "bind", "b", ":10101", "Default URI on which pilosa should listen.")
+	flags.StringVarP(&srv.Config.InternalPort, "internal-port", "", "", "Port to which pilosa should bind for internal state sharing.")
 	flags.IntVarP(&srv.Config.MaxWritesPerRequest, "max-writes-per-request", "", srv.Config.MaxWritesPerRequest, "Number of write commands per request.")
 	flags.IntVarP(&srv.Config.Cluster.ReplicaN, "cluster.replicas", "", 1, "Number of hosts each piece of data should be stored on.")
 	flags.StringSliceVarP(&srv.Config.Cluster.Hosts, "cluster.hosts", "", []string{}, "Comma separated list of hosts in cluster.")
@@ -39,7 +40,6 @@ func BuildServerFlags(cmd *cobra.Command, srv *server.Command) {
 	flags.DurationVarP(&srv.CPUTime, "profile.cpu-time", "", 30*time.Second, "CPU profile duration.")
 	flags.StringVarP(&srv.Config.Cluster.Type, "cluster.type", "", "static", "Determine how the cluster handles membership and state sharing. Choose from [static, http, gossip]")
 	flags.StringVarP(&srv.Config.Cluster.GossipSeed, "cluster.gossip-seed", "", "", "Host with which to seed the gossip membership.")
-	flags.StringVarP(&srv.Config.Cluster.InternalPort, "cluster.internal-port", "", "", "Port to which pilosa should bind for internal state sharing.")
 	flags.StringVarP(&srv.Config.Metric.Service, "metric.service", "", "nop", "Default URI on which pilosa should listen.")
 	flags.StringVarP(&srv.Config.Metric.Host, "metric.host", "", "", "Default URI to send metrics.")
 	flags.DurationVarP((*time.Duration)(&srv.Config.Metric.PollInterval), "metric.poll-interval", "", time.Minute*0, "Polling interval metrics.")

ctl/server_test.go

@@ -16,9 +16,10 @@ package ctl
 
 import (
 	"bytes"
+	"testing"
+
 	"github.com/pilosa/pilosa/server"
 	"github.com/spf13/cobra"
-	"testing"
 )
 
 func TestBuildServerFlags(t *testing.T) {
@@ -27,8 +28,8 @@ func TestBuildServerFlags(t *testing.T) {
 	stdin, stdout, stderr := GetIO(buf)
 	Server := server.NewCommand(stdin, stdout, stderr)
 	BuildServerFlags(cm, Server)
-	if cm.Flags().Lookup("cluster.internal-port").Name == "" {
-		t.Fatal("cluster.internal-port flag is missed ")
+	if cm.Flags().Lookup("internal-port").Name == "" {
+		t.Fatal("internal-port flag is missed ")
 	}
 	if cm.Flags().Lookup("data-dir").Name == "" {
 		t.Fatal("data-dir flag is missed ")

docs/configuration.md

@@ -58,6 +58,17 @@ Any flag that has a value that is a comma separated list on the command line bec
     bind = localhost:10101
     ```
 
+#### Internal Port
+
+* Description: Port to which Pilosa should bind for internal communication.
+* Flag: `--internal-port=11101`
+* Env: `PILOSA_INTERNAL_PORT=11101`
+* Config:
+
+    ```toml
+    internal-port = 11101
+    ```
+
 #### Cluster Hosts
 
 * Description: List of hosts in the cluster. Multiple hosts should be comma separated in the flag and env forms.
@@ -82,18 +93,6 @@ Any flag that has a value that is a comma separated list on the command line bec
     internal-hosts = ["localhost:11101"]
     ```
 
-#### Cluster Internal Port
-
-* Description: Port to which Pilosa should bind for internal communication.
-* Flag: `--cluster.internal-port=11101`
-* Env: `PILOSA_CLUSTER.INTERNAL_PORT=11101`
-* Config:
-
-    ```toml
-    [cluster]
-    internal-port = 11101
-    ```
-
 #### Cluster Poll Interval
 
 * Description: Polling interval for cluster.

pilosa.go

@@ -65,7 +65,6 @@ var (
 	ErrConfigBroadcastPort      = errors.New("internal-port not found in internal-hosts")
 	ErrConfigHostsMismatch      = errors.New("hosts and internal-hosts length mismatch")
 	ErrConfigReplicaNInvalid    = errors.New("replica number must be <= hosts")
-	ErrConfigGossipSeed         = errors.New("invalid gossip seed")
 )
 
 // Regular expression to validate index and frame names.
@@ -143,7 +142,7 @@ func ValidateLabel(label string) error {
 	return nil
 }
 
-// StringInSlice checks is substring a is in the slice
+// StringInSlice checks for substring a in the slice.
 func StringInSlice(a string, list []string) bool {
 	for _, b := range list {
 		if b == a {
@@ -153,7 +152,7 @@ func StringInSlice(a string, list []string) bool {
 	return false
 }
 
-// ContainsSubstring checks is substring a is contained in the slice
+// ContainsSubstring checks to see if substring a is contained in any string in the slice.
 func ContainsSubstring(a string, list []string) bool {
 	for _, b := range list {
 		if strings.Contains(b, a) {

server/server.go

@@ -153,8 +153,8 @@ func (m *Command) SetupServer() error {
 
 	// Set internal port (string).
 	internalPortStr := pilosa.DefaultInternalPort
-	if m.Config.Cluster.InternalPort != "" {
-		internalPortStr = m.Config.Cluster.InternalPort
+	if m.Config.InternalPort != "" {
+		internalPortStr = m.Config.InternalPort
 	}
 
 	switch m.Config.Cluster.Type {