From dee700741aa9694e1e3aa7605303a0714dd1bd0a Mon Sep 17 00:00:00 2001 From: Maxton Huff Date: Thu, 10 Dec 2020 10:51:31 -0600 Subject: [PATCH] deprecate cluster.long-query-time and create long-query-time moved lonquerytime from cluster into server and moved cluster.longquerytime into top level config kept cluster.longquerytime for backwards compatibility, favored if both longquerytime options are present --- api.go | 5 +-- cluster.go | 4 --- cmd/server_test.go | 73 ++++++++++++++++++++++++++++++++++++++++++- ctl/server.go | 3 +- docs/configuration.md | 34 +++++++++++++------- server.go | 4 ++- server/config.go | 7 +++-- server/server.go | 8 ++++- 8 files changed, 112 insertions(+), 26 deletions(-) diff --git a/api.go b/api.go index 57a7b4032..67f8bffe5 100644 --- a/api.go +++ b/api.go @@ -1652,10 +1652,7 @@ func (api *API) StatsWithTags(tags []string) stats.StatsClient { // LongQueryTime returns the configured threshold for logging/statting // long running queries. func (api *API) LongQueryTime() time.Duration { - if api.cluster == nil { - return 0 - } - return api.cluster.longQueryTime + return api.server.longQueryTime } func (api *API) validateShardOwnership(indexName string, shard uint64) error { diff --git a/cluster.go b/cluster.go index 765ee7845..dbf2ad67b 100644 --- a/cluster.go +++ b/cluster.go @@ -211,10 +211,6 @@ type cluster struct { // nolint: maligned // Human-readable name of the cluster. Name string - // Threshold for logging long-running queries - // TODO(2.0) move this out of cluster. (why is it here??) - longQueryTime time.Duration - // Maximum number of Set() or Clear() commands per request. maxWritesPerRequest int diff --git a/cmd/server_test.go b/cmd/server_test.go index bbdd9a79c..ab7541c84 100644 --- a/cmd/server_test.go +++ b/cmd/server_test.go @@ -45,6 +45,7 @@ func TestServerConfig(t *testing.T) { args: []string{"server", "--data-dir", actualDataDir, "--cluster.hosts", "localhost:42454,localhost:10110", "--bind", "localhost:42454", "--bind-grpc", "localhost:30112", "--translation.map-size", "100000"}, env: map[string]string{ "PILOSA_DATA_DIR": "/tmp/myEnvDatadir", + "PILOSA_LONG_QUERY_TIME": "1m30s", "PILOSA_CLUSTER_LONG_QUERY_TIME": "1m30s", "PILOSA_MAX_WRITES_PER_REQUEST": "2000", "PILOSA_PROFILE_BLOCK_RATE": "9123", @@ -55,7 +56,8 @@ func TestServerConfig(t *testing.T) { bind = "localhost:0" bind-grpc = "localhost:0" max-writes-per-request = 3000 - + long-query-time = "1m10s" + [cluster] disabled = true replicas = 2 @@ -73,6 +75,7 @@ func TestServerConfig(t *testing.T) { v.Check(cmd.Server.Config.Bind, "localhost:42454") v.Check(cmd.Server.Config.Cluster.ReplicaN, 2) v.Check(cmd.Server.Config.Cluster.Hosts, []string{"localhost:42454", "localhost:10110"}) + v.Check(cmd.Server.Config.LongQueryTime, toml.Duration(time.Second*90)) v.Check(cmd.Server.Config.Cluster.LongQueryTime, toml.Duration(time.Second*90)) v.Check(cmd.Server.Config.MaxWritesPerRequest, 2000) v.Check(cmd.Server.Config.Translation.MapSize, 100000) @@ -191,3 +194,71 @@ func TestServerConfig(t *testing.T) { test.reset() } } +func TestServerConfig_DeprecateLongQueryTime(t *testing.T) { + tests := []commandTest{ + // TEST 0 + { + args: []string{"server", "--long-query-time", "1m10s"}, + env: map[string]string{}, + cfgFileContent: "", + validation: func() error { + v := validator{} + v.Check(cmd.Server.Config.LongQueryTime, toml.Duration(time.Second*70)) + v.Check(toml.Duration(cmd.Server.API.LongQueryTime()), toml.Duration(time.Second*70)) + return v.Error() + }, + }, + // TEST 1 + { + args: []string{"server", "--cluster.long-query-time", "1m20s"}, + env: map[string]string{}, + cfgFileContent: "", + validation: func() error { + v := validator{} + v.Check(cmd.Server.Config.Cluster.LongQueryTime, toml.Duration(time.Second*80)) + v.Check(toml.Duration(cmd.Server.API.LongQueryTime()), toml.Duration(time.Second*80)) + return v.Error() + }, + }, + // TEST 2: Use old value if both are provided because it is the simplest implementation + { + args: []string{"server", "--long-query-time", "50s", "--cluster.long-query-time", "1m30s"}, + env: map[string]string{}, + cfgFileContent: "", + validation: func() error { + v := validator{} + v.Check(cmd.Server.Config.LongQueryTime, toml.Duration(time.Second*50)) + v.Check(toml.Duration(cmd.Server.Config.Cluster.LongQueryTime), toml.Duration(time.Second*90)) + v.Check(toml.Duration(cmd.Server.API.LongQueryTime()), toml.Duration(time.Second*90)) + return v.Error() + }, + }, + } + + // run server tests + for i, test := range tests { + com := test.setupCommand(t) + executed := make(chan struct{}) + var execErr error + go func() { + execErr = com.Execute() + close(executed) + }() + select { + case <-cmd.Server.Started: + case <-executed: + } + if execErr != nil { + t.Fatalf("executing server command: %v", execErr) + } + err := cmd.Server.Close() + failErr(t, err, "closing pilosa server command") + <-executed + failErr(t, execErr, "executing command") + + if err := test.validation(); err != nil { + t.Fatalf("Failed test %d due to: %v", i, err) + } + test.reset() + } +} diff --git a/ctl/server.go b/ctl/server.go index a55bca6ca..187f4998d 100644 --- a/ctl/server.go +++ b/ctl/server.go @@ -36,6 +36,7 @@ func BuildServerFlags(cmd *cobra.Command, srv *server.Command) { flags.BoolVar(&srv.Config.Verbose, "verbose", srv.Config.Verbose, "Enable verbose logging") flags.Uint64Var(&srv.Config.MaxMapCount, "max-map-count", srv.Config.MaxMapCount, "Limits the maximum number of active mmaps. Pilosa will fall back to reading files once this is exhausted. Set below your system's vm.max_map_count.") flags.Uint64Var(&srv.Config.MaxFileCount, "max-file-count", srv.Config.MaxFileCount, "Soft limit on the maximum number of fragment files Pilosa keeps open simultaneously.") + flags.DurationVarP((*time.Duration)(&srv.Config.LongQueryTime), "long-query-time", "", time.Duration(srv.Config.LongQueryTime), "Duration that will trigger log and stat messages for slow queries. Zero to disable.") flags.IntVar(&srv.Config.QueryHistoryLength, "query-history-length", srv.Config.QueryHistoryLength, "Number of queries to remember in history.") // TLS @@ -49,7 +50,7 @@ func BuildServerFlags(cmd *cobra.Command, srv *server.Command) { flags.BoolVarP(&srv.Config.Cluster.Coordinator, "cluster.coordinator", "", srv.Config.Cluster.Coordinator, "Host that will act as cluster coordinator during startup and resizing.") 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. Only used for testing.") - flags.DurationVarP((*time.Duration)(&srv.Config.Cluster.LongQueryTime), "cluster.long-query-time", "", time.Minute, "Duration that will trigger log and stat messages for slow queries.") + flags.DurationVarP((*time.Duration)(&srv.Config.Cluster.LongQueryTime), "cluster.long-query-time", "", time.Duration(srv.Config.Cluster.LongQueryTime), "RENAMED TO 'long-query-time': Duration that will trigger log and stat messages for slow queries.") // negative duration indicates invalid value because 0 is meaningful flags.StringVar(&srv.Config.Cluster.Name, "cluster.name", srv.Config.Cluster.Name, "Human-readable name for the cluster.") // Translation diff --git a/docs/configuration.md b/docs/configuration.md index 57133e666..736dae17f 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -114,6 +114,16 @@ The config file is in the [toml format](https://github.com/toml-lang/toml) and h ```toml verbose = true ``` +#### Long Query Time + +* Description: Duration that will trigger log and stat messages for slow queries. +* Flag: `long-query-time="1m0s"` +* Env: `PILOSA_CLUSTER_LONG_QUERY_TIME="1m0s"` +* Config: + + ```toml + long-query-time = "1m0s" + ``` #### Max Map Count @@ -220,6 +230,18 @@ The config file is in the [toml format](https://github.com/toml-lang/toml) and h key = "/var/secret/gossip.key32" ``` +#### Cluster Long Query Time + +* Description (DEPRICATED, see Long Query Time): Duration that will trigger log and stat messages for slow queries. +* Flag: `cluster.long-query-time="1m0s"` +* Env: `PILOSA_CLUSTER_LONG_QUERY_TIME="1m0s"` +* Config: + + ```toml + [cluster] + long-query-time = "1m0s" + ``` + #### Cluster Coordinator * Description: Indicates whether the node should act as the coordinator for the cluster. Only one node per cluster should be the coordinator. @@ -232,18 +254,6 @@ The config file is in the [toml format](https://github.com/toml-lang/toml) and h coordinator = true ``` -#### Cluster Long Query Time - -* Description: Duration that will trigger log and stat messages for slow queries. -* Flag: `cluster.long-query-time="1m0s"` -* Env: `PILOSA_CLUSTER_LONG_QUERY_TIME="1m0s"` -* Config: - - ```toml - [cluster] - long-query-time = "1m0s" - ``` - #### Cluster Replicas * Description: Number of hosts each piece of data should be stored on. diff --git a/server.go b/server.go index 4d77eb195..6fc01ebe7 100644 --- a/server.go +++ b/server.go @@ -87,6 +87,8 @@ type Server struct { // nolint: maligned defaultClient InternalClient dataDir string + // Threshold for logging long-running queries + longQueryTime time.Duration queryHistoryLength int } @@ -149,7 +151,7 @@ func OptServerAntiEntropyInterval(interval time.Duration) ServerOption { // used to set long query duration. func OptServerLongQueryTime(dur time.Duration) ServerOption { return func(s *Server) error { - s.cluster.longQueryTime = dur + s.longQueryTime = dur return nil } } diff --git a/server/config.go b/server/config.go index f6b98e64e..59f390210 100644 --- a/server/config.go +++ b/server/config.go @@ -124,10 +124,11 @@ type Config struct { ReplicaN int `toml:"replicas"` Hosts []string `toml:"hosts"` Name string `toml:"name"` - // TODO(2.0) move this out of cluster. (why is it here??) + // This LongQueryTime is deprecated but still exists for backward compatibility LongQueryTime toml.Duration `toml:"long-query-time"` } `toml:"cluster"` + LongQueryTime toml.Duration `toml:"long-query-time"` // Gossip config is based around memberlist.Config. Gossip gossip.Config `toml:"gossip"` @@ -239,13 +240,15 @@ func NewConfig() *Config { RBFConfig: rbfcfg.NewDefaultConfig(), QueryHistoryLength: 100, + + LongQueryTime: toml.Duration(-time.Minute), } // Cluster config. c.Cluster.Disabled = false c.Cluster.ReplicaN = 1 c.Cluster.Hosts = []string{} - c.Cluster.LongQueryTime = toml.Duration(time.Minute) + c.Cluster.LongQueryTime = toml.Duration(-time.Minute) //TODO remove this once cluster.longQueryTime is fully deprecated // Gossip config. c.Gossip.Port = "14000" diff --git a/server/server.go b/server/server.go index 4ffa3c72c..291c986e1 100644 --- a/server/server.go +++ b/server/server.go @@ -380,6 +380,12 @@ func (m *Command) SetupServer() error { if m.Config.Translation.PrimaryURL != "" { m.logger.Printf("DEPRECATED: The primary-url configuration option is no longer used.") } + // Handle renamed and deprecated config parameter + longQueryTime := m.Config.LongQueryTime + if m.Config.Cluster.LongQueryTime >= 0 { + longQueryTime = m.Config.Cluster.LongQueryTime + m.logger.Printf("DEPRECATED: Configuration parameter cluster.long-query-time has been renamed to long-query-time") + } // Set Coordinator. coordinatorOpt := pilosa.OptServerIsCoordinator(false) @@ -389,7 +395,7 @@ func (m *Command) SetupServer() error { serverOptions := []pilosa.ServerOption{ pilosa.OptServerAntiEntropyInterval(time.Duration(m.Config.AntiEntropy.Interval)), - pilosa.OptServerLongQueryTime(time.Duration(m.Config.Cluster.LongQueryTime)), + pilosa.OptServerLongQueryTime(time.Duration(longQueryTime)), pilosa.OptServerDataDir(m.Config.DataDir), pilosa.OptServerReplicaN(m.Config.Cluster.ReplicaN), pilosa.OptServerMaxWritesPerRequest(m.Config.MaxWritesPerRequest),