From a1cc4c5540501f570db0b46b6f8adfed788b86e1 Mon Sep 17 00:00:00 2001 From: Nikhil Benesch Date: Tue, 28 Aug 2018 23:48:51 -0400 Subject: [PATCH 1/2] Revert "storage: enable the merge queue by default" This reverts commit 98ca1d0fe35431a26c6cbfbcf9d7b581d1373c1f. The merge queue will be reenabled once flaky tests are fixed. --- docs/generated/settings/settings.html | 2 +- pkg/storage/merge_queue.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/generated/settings/settings.html b/docs/generated/settings/settings.html index ec32f20fbdca..10fa5d7b8d44 100644 --- a/docs/generated/settings/settings.html +++ b/docs/generated/settings/settings.html @@ -31,7 +31,7 @@ kv.raft_log.synchronizebooleantrueset to true to synchronize on Raft log writes to persistent storage ('false' risks data loss) kv.range.backpressure_range_size_multiplierfloat2multiple of range_max_bytes that a range is allowed to grow to without splitting before writes to that range are blocked, or 0 to disable kv.range_descriptor_cache.sizeinteger1000000maximum number of entries in the range descriptor and leaseholder caches -kv.range_merge.queue_enabledbooleantruewhether the automatic merge queue is enabled +kv.range_merge.queue_enabledbooleanfalsewhether the automatic merge queue is enabled kv.rangefeed.enabledbooleanfalseif set, rangefeed registration is enabled kv.snapshot_rebalance.max_ratebyte size2.0 MiBthe rate limit (bytes/sec) to use for rebalance snapshots kv.snapshot_recovery.max_ratebyte size8.0 MiBthe rate limit (bytes/sec) to use for recovery snapshots diff --git a/pkg/storage/merge_queue.go b/pkg/storage/merge_queue.go index ef97073ebe5a..8ab8b91ac83a 100644 --- a/pkg/storage/merge_queue.go +++ b/pkg/storage/merge_queue.go @@ -49,7 +49,7 @@ const ( var MergeQueueEnabled = settings.RegisterBoolSetting( "kv.range_merge.queue_enabled", "whether the automatic merge queue is enabled", - true, + false, ) // MergeQueueInterval is a setting that controls how often the merge queue waits From f932530e62cb1147a9ba7798eb09fa9fd52c441c Mon Sep 17 00:00:00 2001 From: Peter Mattis Date: Tue, 28 Aug 2018 22:07:02 -0400 Subject: [PATCH 2/2] server: deflake TestRapidRestarts In go1.10 and earlier, it was not safe to call `http.ServeMux.ServeHTTP` concurrently with `http.ServeMux.Handle`. (This is fixed in go1.11). In the interim, provide our own safeServeMux wrapper that provides proper locking. Fixes #29227 Release note: None --- pkg/server/servemux_test.go | 66 +++++++++++++++++++++++++++++++++++++ pkg/server/server.go | 26 +++++++++++++-- 2 files changed, 90 insertions(+), 2 deletions(-) create mode 100644 pkg/server/servemux_test.go diff --git a/pkg/server/servemux_test.go b/pkg/server/servemux_test.go new file mode 100644 index 000000000000..71fdee276f65 --- /dev/null +++ b/pkg/server/servemux_test.go @@ -0,0 +1,66 @@ +// Copyright 2018 The Cockroach Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +// implied. See the License for the specific language governing +// permissions and limitations under the License. + +package server + +import ( + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "sync" + "testing" + "time" + + "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/cockroachdb/cockroach/pkg/util/timeutil" +) + +func TestServeMuxConcurrency(t *testing.T) { + defer leaktest.AfterTest(t)() + + const duration = 20 * time.Millisecond + start := timeutil.Now() + + // TODO(peter): This test reliably fails using http.ServeMux with a + // "concurrent map read and write error" on go1.10. The bug in http.ServeMux + // is fixed in go1.11. + var mux safeServeMux + var wg sync.WaitGroup + wg.Add(2) + + go func() { + defer wg.Done() + f := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}) + for i := 1; timeutil.Since(start) < duration; i++ { + mux.Handle(fmt.Sprintf("/%d", i), f) + } + }() + + go func() { + defer wg.Done() + for i := 1; timeutil.Since(start) < duration; i++ { + r := &http.Request{ + Method: "GET", + URL: &url.URL{ + Path: "/", + }, + } + w := httptest.NewRecorder() + mux.ServeHTTP(w, r) + } + }() + + wg.Wait() +} diff --git a/pkg/server/server.go b/pkg/server/server.go index 26e29cd876c9..9e6765eec20d 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -80,6 +80,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/protoutil" "github.com/cockroachdb/cockroach/pkg/util/retry" "github.com/cockroachdb/cockroach/pkg/util/stop" + "github.com/cockroachdb/cockroach/pkg/util/syncutil" "github.com/cockroachdb/cockroach/pkg/util/timeutil" "github.com/cockroachdb/cockroach/pkg/util/uuid" ) @@ -123,13 +124,35 @@ var ( ) ) +// TODO(peter): Until go1.11, ServeMux.ServeHTTP was not safe to call +// concurrently with ServeMux.Handle. So we provide our own wrapper with proper +// locking. Slightly less efficient because it locks unnecessarily, but +// safe. See TestServeMuxConcurrency. Should remove once we've upgraded to +// go1.11. +type safeServeMux struct { + mu syncutil.RWMutex + mux http.ServeMux +} + +func (mux *safeServeMux) Handle(pattern string, handler http.Handler) { + mux.mu.Lock() + mux.mux.Handle(pattern, handler) + mux.mu.Unlock() +} + +func (mux *safeServeMux) ServeHTTP(w http.ResponseWriter, r *http.Request) { + mux.mu.RLock() + mux.mux.ServeHTTP(w, r) + mux.mu.RUnlock() +} + // Server is the cockroach server node. type Server struct { nodeIDContainer base.NodeIDContainer cfg Config st *cluster.Settings - mux *http.ServeMux + mux safeServeMux clock *hlc.Clock rpcContext *rpc.Context grpc *grpc.Server @@ -182,7 +205,6 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) { clock := hlc.NewClock(hlc.UnixNano, time.Duration(cfg.MaxOffset)) s := &Server{ st: st, - mux: http.NewServeMux(), clock: clock, stopper: stopper, cfg: cfg,