From 3f303d1098e6173d9f0af0bc031772f99525d67d Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Thu, 12 Apr 2018 19:52:48 -0500 Subject: [PATCH 1/3] remove global defaults from config.go these were occaisionally referenced elsewhere in the codebase - in all but one case, there were workarounds that are actually better I think. In the one case there wasn't I created a single top level DefaultConfig object which is instantiated with all the default values and can be referred to if necessary. There was a bug in fragment.go with the way MaxWritesPerRequest was treated if it was 0. Elsewhere, 0 meant no limit, but here, it would have caused a division by 0. Changed the default metrics provider from "nop" to "none", although "nop" will still work. Previously, any value other than "statsd" or "expvar" was treated as "nop", but I've changed this behavior to return an error if an invalid string is provided. I think this is better behavior, because in the case that someone bothered to change the default, they were probably interested in actually getting stats, and might be annoyed when it silently failed. --- cluster.go | 9 +- config.go | 215 ++++++++++++++++-------------------- config_test.go | 2 +- ctl/generate_config_test.go | 3 +- ctl/import_test.go | 2 +- docs/configuration.md | 2 +- fragment.go | 10 +- server.go | 5 +- server/server.go | 14 +-- 9 files changed, 118 insertions(+), 144 deletions(-) diff --git a/cluster.go b/cluster.go index abf753892..c7945d938 100644 --- a/cluster.go +++ b/cluster.go @@ -275,11 +275,10 @@ type Cluster struct { // NewCluster returns a new instance of Cluster with defaults. func NewCluster() *Cluster { return &Cluster{ - Hasher: &jmphasher{}, - PartitionN: DefaultPartitionN, - ReplicaN: DefaultReplicaN, - MaxWritesPerRequest: DefaultMaxWritesPerRequest, - EventReceiver: NopEventReceiver, + Hasher: &jmphasher{}, + PartitionN: DefaultPartitionN, + ReplicaN: DefaultReplicaN, + EventReceiver: NopEventReceiver, joiningLeavingNodes: make(chan nodeAction, 10), // buffered channel jobs: make(map[int64]*ResizeJob), diff --git a/config.go b/config.go index 84f199dc3..c03f03ac3 100644 --- a/config.go +++ b/config.go @@ -25,93 +25,8 @@ const ( ClusterGossip = "gossip" ) -const ( - // DefaultDataDir is the default data directory. - DefaultDataDir = "~/.pilosa" - - // DefaultHost is the default hostname to use. - DefaultHost = "localhost" - - // DefaultPort is the default port to use with the hostname. - DefaultPort = "10101" - - // DefaultClusterDisabled sets the node intercommunication method. - DefaultClusterDisabled = false - - // DefaultMetrics sets the internal metrics to no-op. - DefaultMetrics = "nop" - - // DefaultMaxWritesPerRequest is the default number of writes per request. - DefaultMaxWritesPerRequest = 5000 - - // Gossip config based on memberlist.Config. - - // Port indicates the port to which pilosa should bind for internal state sharing. - DefaultGossipPort = "14000" - - // StreamTimeout is the timeout for establishing a stream connection with - // a remote node for a full state sync, and for stream read and write - // operations. Maps to memberlist TCPTimeout. - DefaultGossipStreamTimeout = 10 * time.Second - - // SuspicionMult is the multiplier for determining the time an - // inaccessible node is considered suspect before declaring it dead. - // The actual timeout is calculated using the formula: - // - // SuspicionTimeout = SuspicionMult * log(N+1) * ProbeInterval - // - // This allows the timeout to scale properly with expected propagation - // delay with a larger cluster size. The higher the multiplier, the longer - // an inaccessible node is considered part of the cluster before declaring - // it dead, giving that suspect node more time to refute if it is indeed - // still alive. - DefaultGossipSuspicionMult = 4 - - // PushPullInterval is the interval between complete state syncs. - // Complete state syncs are done with a single node over TCP and are - // quite expensive relative to standard gossiped messages. Setting this - // to zero will disable state push/pull syncs completely. - // - // Setting this interval lower (more frequent) will increase convergence - // speeds across larger clusters at the expense of increased bandwidth - // usage. - DefaultGossipPushPullInterval = 30 * time.Second - - // ProbeInterval and ProbeTimeout are used to configure probing behavior - // for memberlist. - // - // ProbeInterval is the interval between random node probes. Setting - // this lower (more frequent) will cause the memberlist cluster to detect - // failed nodes more quickly at the expense of increased bandwidth usage. - // - // ProbeTimeout is the timeout to wait for an ack from a probed node - // before assuming it is unhealthy. This should be set to 99-percentile - // of RTT (round-trip time) on your network. - DefaultGossipProbeInterval = 1 * time.Second - DefaultGossipProbeTimeout = 500 * time.Millisecond - - // Interval and Nodes are used to configure the gossip - // behavior of memberlist. - // - // Interval is the interval between sending messages that need - // to be gossiped that haven't been able to piggyback on probing messages. - // If this is set to zero, non-piggyback gossip is disabled. By lowering - // this value (more frequent) gossip messages are propagated across - // the cluster more quickly at the expense of increased bandwidth. - // - // Nodes is the number of random nodes to send gossip messages to - // per Interval. Increasing this number causes the gossip messages - // to propagate across the cluster more quickly at the expense of - // increased bandwidth. - // - // ToTheDeadTime is the interval after which a node has died that - // we will still try to gossip to it. This gives it a chance to refute. - DefaultGossipInterval = 200 * time.Millisecond - DefaultGossipNodes = 3 - DefaultGossipToTheDeadTime = 30 * time.Second - - DefaultMetricPollInterval = 0 * time.Minute -) +// DefaultConfig is a Config structure that holds all the default values. +var DefaultConfig = NewConfig() // TLSConfig contains TLS configuration type TLSConfig struct { @@ -125,20 +40,27 @@ type TLSConfig struct { // Config represents the configuration for the command. type Config struct { + // DataDir is the default data directory. DataDir string `toml:"data-dir"` - Bind string `toml:"bind"` + // Bind is the host:port on which Pilosa will listen. + Bind string `toml:"bind"` - // Limits the number of mutating commands that can be in a single request to - // the server. This includes SetBit, ClearBit, SetRowAttrs & SetColumnAttrs. + // MaxWritesPerRequest limits the number of mutating commands that can be in + // a single request to the server. This includes SetBit, ClearBit, + // SetRowAttrs & SetColumnAttrs. MaxWritesPerRequest int `toml:"max-writes-per-request"` + // LogPath configures where Pilosa will write logs. LogPath string `toml:"log-path"` - Verbose bool `toml:"verbose"` + + // Verbose toggles verbose logging which can be useful for debugging. + Verbose bool `toml:"verbose"` // TLS TLS TLSConfig Cluster struct { + // Disabled controls whether clustering functionality is enabled. Disabled bool `toml:"disabled"` Coordinator bool `toml:"coordinator"` ReplicaN int `toml:"replicas"` @@ -146,18 +68,69 @@ type Config struct { LongQueryTime Duration `toml:"long-query-time"` } `toml:"cluster"` + // Gossip config is based around memberlist.Config. Gossip struct { - Port string `toml:"port"` - Seeds []string `toml:"seeds"` - Key string `toml:"key"` - StreamTimeout Duration `toml:"stream-timeout"` - SuspicionMult int `toml:"suspicion-mult"` + // Port indicates the port to which pilosa should bind for internal state sharing. + Port string `toml:"port"` + Seeds []string `toml:"seeds"` + Key string `toml:"key"` + // StreamTimeout is the timeout for establishing a stream connection with + // a remote node for a full state sync, and for stream read and write + // operations. Maps to memberlist TCPTimeout. + StreamTimeout Duration `toml:"stream-timeout"` + // SuspicionMult is the multiplier for determining the time an + // inaccessible node is considered suspect before declaring it dead. + // The actual timeout is calculated using the formula: + // + // SuspicionTimeout = SuspicionMult * log(N+1) * ProbeInterval + // + // This allows the timeout to scale properly with expected propagation + // delay with a larger cluster size. The higher the multiplier, the longer + // an inaccessible node is considered part of the cluster before declaring + // it dead, giving that suspect node more time to refute if it is indeed + // still alive. + SuspicionMult int `toml:"suspicion-mult"` + // PushPullInterval is the interval between complete state syncs. + // Complete state syncs are done with a single node over TCP and are + // quite expensive relative to standard gossiped messages. Setting this + // to zero will disable state push/pull syncs completely. + // + // Setting this interval lower (more frequent) will increase convergence + // speeds across larger clusters at the expense of increased bandwidth + // usage. PushPullInterval Duration `toml:"push-pull-interval"` - ProbeTimeout Duration `toml:"probe-timeout"` - ProbeInterval Duration `toml:"probe-interval"` - Nodes int `toml:"nodes"` - Interval Duration `toml:"interval"` - ToTheDeadTime Duration `toml:"to-the-dead-time"` + // ProbeInterval and ProbeTimeout are used to configure probing behavior + // for memberlist. + // + // ProbeInterval is the interval between random node probes. Setting + // this lower (more frequent) will cause the memberlist cluster to detect + // failed nodes more quickly at the expense of increased bandwidth usage. + // + // ProbeTimeout is the timeout to wait for an ack from a probed node + // before assuming it is unhealthy. This should be set to 99-percentile + // of RTT (round-trip time) on your network. + ProbeInterval Duration `toml:"probe-interval"` + ProbeTimeout Duration `toml:"probe-timeout"` + + // Interval and Nodes are used to configure the gossip + // behavior of memberlist. + // + // Interval is the interval between sending messages that need + // to be gossiped that haven't been able to piggyback on probing messages. + // If this is set to zero, non-piggyback gossip is disabled. By lowering + // this value (more frequent) gossip messages are propagated across + // the cluster more quickly at the expense of increased bandwidth. + // + // Nodes is the number of random nodes to send gossip messages to + // per Interval. Increasing this number causes the gossip messages + // to propagate across the cluster more quickly at the expense of + // increased bandwidth. + // + // ToTheDeadTime is the interval after which a node has died that + // we will still try to gossip to it. This gives it a chance to refute. + Interval Duration `toml:"interval"` + Nodes int `toml:"nodes"` + ToTheDeadTime Duration `toml:"to-the-dead-time"` } `toml:"gossip"` AntiEntropy struct { @@ -165,51 +138,55 @@ type Config struct { } `toml:"anti-entropy"` Metric struct { - Service string `toml:"service"` + // Service can be statsd, expvar, or none. + Service string `toml:"service"` + // Host tells the statsd client where to write. Host string `toml:"host"` PollInterval Duration `toml:"poll-interval"` - Diagnostics bool `toml:"diagnostics"` + // Diagnostics toggles sending some limited diagnostic information to + // Pilosa's developers. + Diagnostics bool `toml:"diagnostics"` } `toml:"metric"` } // NewConfig returns an instance of Config with default options. func NewConfig() *Config { c := &Config{ - DataDir: DefaultDataDir, - Bind: ":" + DefaultPort, - MaxWritesPerRequest: DefaultMaxWritesPerRequest, + DataDir: "~/.pilosa", + Bind: ":10101", + MaxWritesPerRequest: 5000, // LogPath: "", // Verbose: false, TLS: TLSConfig{}, } // Cluster config. - c.Cluster.Disabled = DefaultClusterDisabled + c.Cluster.Disabled = false // c.Cluster.Coordinator = false c.Cluster.ReplicaN = DefaultReplicaN c.Cluster.Hosts = []string{} c.Cluster.LongQueryTime = Duration(time.Minute) // Gossip config. - // c.Gossip.Port = "" + c.Gossip.Port = "14000" // c.Gossip.Seeds = []string{} // c.Gossip.Key = "" - c.Gossip.StreamTimeout = Duration(DefaultGossipStreamTimeout) - c.Gossip.SuspicionMult = DefaultGossipSuspicionMult - c.Gossip.PushPullInterval = Duration(DefaultGossipPushPullInterval) - c.Gossip.ProbeTimeout = Duration(DefaultGossipProbeTimeout) - c.Gossip.ProbeInterval = Duration(DefaultGossipProbeInterval) - c.Gossip.Nodes = DefaultGossipNodes - c.Gossip.Interval = Duration(DefaultGossipInterval) - c.Gossip.ToTheDeadTime = Duration(DefaultGossipToTheDeadTime) + c.Gossip.StreamTimeout = Duration(10 * time.Second) + c.Gossip.SuspicionMult = 4 + c.Gossip.PushPullInterval = Duration(30 * time.Second) + c.Gossip.ProbeInterval = Duration(1 * time.Second) + c.Gossip.ProbeTimeout = Duration(500 * time.Millisecond) + c.Gossip.Interval = Duration(200 * time.Millisecond) + c.Gossip.Nodes = 3 + c.Gossip.ToTheDeadTime = Duration(30 * time.Second) // AntiEntropy config. - c.AntiEntropy.Interval = Duration(DefaultAntiEntropyInterval) + c.AntiEntropy.Interval = Duration(10 * time.Minute) // Metric config. - c.Metric.Service = DefaultMetrics + c.Metric.Service = "none" // c.Metric.Host = "" - c.Metric.PollInterval = Duration(DefaultMetricPollInterval) + c.Metric.PollInterval = Duration(0 * time.Minute) c.Metric.Diagnostics = true return c diff --git a/config_test.go b/config_test.go index 1359e2e31..c07486ddb 100644 --- a/config_test.go +++ b/config_test.go @@ -25,7 +25,7 @@ import ( func Test_NewConfig(t *testing.T) { c := pilosa.NewConfig() - if c.Cluster.Disabled != pilosa.DefaultClusterDisabled { + if c.Cluster.Disabled { t.Fatalf("unexpected Cluster.Disabled: %v", c.Cluster.Disabled) } diff --git a/ctl/generate_config_test.go b/ctl/generate_config_test.go index 801ab4055..56f392bba 100644 --- a/ctl/generate_config_test.go +++ b/ctl/generate_config_test.go @@ -17,7 +17,6 @@ package ctl import ( "bytes" "context" - "github.com/pilosa/pilosa" "io" "os" "strings" @@ -35,7 +34,7 @@ func TestGenerateConfigCommand_Run(t *testing.T) { io.Copy(&buf, r) if err != nil { t.Fatalf("Config Run doesn't work: %s", err) - } else if !strings.Contains(buf.String(), pilosa.DefaultHost) { + } else if !strings.Contains(buf.String(), "localhost:10101") { t.Fatalf("Unexpected config: %s", buf.String()) } } diff --git a/ctl/import_test.go b/ctl/import_test.go index bd259e342..55ae6d243 100644 --- a/ctl/import_test.go +++ b/ctl/import_test.go @@ -134,7 +134,7 @@ func TestImportCommand_InvalidFile(t *testing.T) { buf := bytes.Buffer{} stdin, stdout, stderr := GetIO(buf) cm := NewImportCommand(stdin, stdout, stderr) - cm.Host = pilosa.DefaultHost + cm.Host = "anyhost" cm.Index = "i" cm.Frame = "f" file, err := ioutil.TempFile("", "import.csv") diff --git a/docs/configuration.md b/docs/configuration.md index 4d71dee8e..2528299a4 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -213,7 +213,7 @@ The config file is in the [toml format](https://github.com/toml-lang/toml) and h ``` #### Metric Service -* Description: Which stats service to use. Choose from [statsd, expvar]. +* Description: Which stats service to use. Choose from [statsd, expvar, none]. * Flag: `--metric.service=statsd` * Env: `PILOSA_METRIC_SERVICE=statsd' * Config: diff --git a/fragment.go b/fragment.go index c44a07380..56342690c 100644 --- a/fragment.go +++ b/fragment.go @@ -1832,15 +1832,19 @@ func (s *FragmentSyncer) syncBlock(id int) error { // Generate query with sets & clears, and group the requests to not exceed MaxWritesPerRequest. total := len(set.ColumnIDs) + len(clear.ColumnIDs) - buffers := make([]bytes.Buffer, int(math.Ceil(float64(total)/float64(s.Cluster.MaxWritesPerRequest)))) + maxWrites := s.Cluster.MaxWritesPerRequest + if maxWrites <= 0 { + maxWrites = 5000 + } + buffers := make([]bytes.Buffer, int(math.Ceil(float64(total)/float64(maxWrites)))) // Only sync the standard block. for j := 0; j < len(set.ColumnIDs); j++ { - fmt.Fprintf(&(buffers[count/s.Cluster.MaxWritesPerRequest]), "SetBit(frame=%q, row=%d, col=%d)\n", f.Frame(), set.RowIDs[j], (f.Slice()*SliceWidth)+set.ColumnIDs[j]) + fmt.Fprintf(&(buffers[count/maxWrites]), "SetBit(frame=%q, row=%d, col=%d)\n", f.Frame(), set.RowIDs[j], (f.Slice()*SliceWidth)+set.ColumnIDs[j]) count++ } for j := 0; j < len(clear.ColumnIDs); j++ { - fmt.Fprintf(&(buffers[count/s.Cluster.MaxWritesPerRequest]), "ClearBit(frame=%q, row=%d, col=%d)\n", f.Frame(), clear.RowIDs[j], (f.Slice()*SliceWidth)+clear.ColumnIDs[j]) + fmt.Fprintf(&(buffers[count/maxWrites]), "ClearBit(frame=%q, row=%d, col=%d)\n", f.Frame(), clear.RowIDs[j], (f.Slice()*SliceWidth)+clear.ColumnIDs[j]) count++ } diff --git a/server.go b/server.go index 21f974ad8..57ae423df 100644 --- a/server.go +++ b/server.go @@ -37,8 +37,7 @@ import ( // Default server settings. const ( - DefaultAntiEntropyInterval = 10 * time.Minute - DefaultDiagnosticServer = "https://diagnostics.pilosa.com/v0/diagnostics" + DefaultDiagnosticServer = "https://diagnostics.pilosa.com/v0/diagnostics" ) // Ensure Server implements interfaces. @@ -108,7 +107,7 @@ func NewServer() *Server { NewAttrStore: NewNopAttrStore, - AntiEntropyInterval: DefaultAntiEntropyInterval, + AntiEntropyInterval: time.Duration(DefaultConfig.AntiEntropy.Interval), MetricInterval: 0, DiagnosticInterval: 0, diff --git a/server/server.go b/server/server.go index 62c7a70a5..7f66d31ed 100644 --- a/server/server.go +++ b/server/server.go @@ -20,7 +20,6 @@ package server import ( - "errors" "fmt" "io" "log" @@ -40,6 +39,7 @@ import ( "github.com/pilosa/pilosa/gossip" "github.com/pilosa/pilosa/statik" "github.com/pilosa/pilosa/statsd" + "github.com/pkg/errors" ) func init() { @@ -255,13 +255,7 @@ func (m *Command) SetupNetworking() error { return nil } - // Set internal port (string). - gossipPortStr := pilosa.DefaultGossipPort - if m.Config.Gossip.Port != "" { - gossipPortStr = m.Config.Gossip.Port - } - - gossipPort, err := strconv.Atoi(gossipPortStr) + gossipPort, err := strconv.Atoi(m.Config.Gossip.Port) if err != nil { return err } @@ -318,7 +312,9 @@ func NewStatsClient(name string, host string) (pilosa.StatsClient, error) { return pilosa.NewExpvarStatsClient(), nil case "statsd": return statsd.NewStatsClient(host) - default: + case "nop", "none": return pilosa.NopStatsClient, nil + default: + return nil, errors.Errorf("'%v' not a valid stats client, choose from [expvar, statsd, none].") } } From 0c35094bfc43b311239fc1da47a550445ebde44b Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Fri, 13 Apr 2018 10:21:47 -0500 Subject: [PATCH 2/3] few more docs tweaks --- docs/configuration.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 2528299a4..2d9d109b9 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -197,12 +197,12 @@ The config file is in the [toml format](https://github.com/toml-lang/toml) and h ```toml [profile] - cpu = "/path/to/somewhere" + cpu = "/path/to/somewhere" ``` #### Profile CPU Time -* Description: Amount of time to collect cpu profiling data if `profile.cpu` is set. +* Description: Amount of time to collect cpu profiling data at startup if `profile.cpu` is set. * Flag: `--profile.cpu-time="30s"` * Env: `PILOSA_PROFILE_CPU_TIME="30s" * Config: @@ -236,7 +236,7 @@ The config file is in the [toml format](https://github.com/toml-lang/toml) and h #### Metric Poll Interval -* Description: Polling interval for runtime metrics. +* Description: Rate at which runtime metrics (such as open file handles and memory usage) are collected. * Flag: `metric.poll-interval=”0m15s”` * Env: `PILOSA_METRIC_POLL_INTERVAL=0m15s` * Config: @@ -248,7 +248,7 @@ The config file is in the [toml format](https://github.com/toml-lang/toml) and h #### Metric Diagnostics -* Description: Enable diagnostic reporting. To disable diagnostics set to false. +* Description: Enable reporting of limited usage statistics to Pilosa developers. To disable, set to false. * Flag: `metric.diagnostics` * Env: `PILOSA_METRIC_DIAGNOSTICS` * Config: From 7233597f35951eb5827cc3d2a059b190213dba4c Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Sun, 15 Apr 2018 17:20:14 -0500 Subject: [PATCH 3/3] remove DefaultConfig global (only used once). Fix data-dir comment --- config.go | 6 ++---- server.go | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/config.go b/config.go index c03f03ac3..483965916 100644 --- a/config.go +++ b/config.go @@ -25,9 +25,6 @@ const ( ClusterGossip = "gossip" ) -// DefaultConfig is a Config structure that holds all the default values. -var DefaultConfig = NewConfig() - // TLSConfig contains TLS configuration type TLSConfig struct { // CertificatePath contains the path to the certificate (.crt or .pem file) @@ -40,7 +37,8 @@ type TLSConfig struct { // Config represents the configuration for the command. type Config struct { - // DataDir is the default data directory. + // DataDir is the directory where Pilosa stores both indexed data and + // running state such as cluster topology information. DataDir string `toml:"data-dir"` // Bind is the host:port on which Pilosa will listen. Bind string `toml:"bind"` diff --git a/server.go b/server.go index 57ae423df..bb629b66a 100644 --- a/server.go +++ b/server.go @@ -107,7 +107,7 @@ func NewServer() *Server { NewAttrStore: NewNopAttrStore, - AntiEntropyInterval: time.Duration(DefaultConfig.AntiEntropy.Interval), + AntiEntropyInterval: time.Duration(NewConfig().AntiEntropy.Interval), MetricInterval: 0, DiagnosticInterval: 0,