Merge pull request #1210 from Maxtonian/lonquerytime2

deprecate cluster.long-query-time and create long-query-time
2026-09-07 17:15:56 +00:00 · 2020-12-10 11:14:09 -06:00 · 2020-12-10 11:14:09 -06:00 · 8c20255b12
commit 8c20255b12
parent 087418566b dee700741a
8 changed files with 112 additions and 26 deletions
--- 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 {
--- 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

--- 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()
+	}
+}
--- 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
--- 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. 
--- 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
 	}
 }
--- 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"
--- 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),