From d8ff91e9d9e1a3d7f29ffdb3ea41be6e7a9ab1a5 Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Thu, 1 Jul 2021 12:58:39 -0400 Subject: [PATCH 1/2] kv: remove EvalContext.Engine This change removes the Engine method from EvalContext. Removing this is important, as its existence appears to undermine #55461 and make #66485 difficult. The first place where this was used was in EndTxn's evaluation function. I don't see any reason for this. In fact, we had a TODO to fix this, which we could have addressed years ago. The second place where this was used was in RecomputeStats's evaluation function. There, it was used for two reasons. First, it was used because `storage.Batch` used to not provide a consistent view of data. They now do. It was also used to evade spanset assertions, which this commit addresses in a better way. --- .../kvserver/batcheval/cmd_end_transaction.go | 9 +++---- .../kvserver/batcheval/cmd_recompute_stats.go | 25 ++++++------------- pkg/kv/kvserver/batcheval/eval_context.go | 5 ---- pkg/kv/kvserver/replica_eval_context_span.go | 6 ----- pkg/kv/kvserver/spanset/batch.go | 13 ++++++++++ 5 files changed, 23 insertions(+), 35 deletions(-) diff --git a/pkg/kv/kvserver/batcheval/cmd_end_transaction.go b/pkg/kv/kvserver/batcheval/cmd_end_transaction.go index fb0d47b432e0..d424770783ee 100644 --- a/pkg/kv/kvserver/batcheval/cmd_end_transaction.go +++ b/pkg/kv/kvserver/batcheval/cmd_end_transaction.go @@ -949,11 +949,8 @@ func splitTriggerHelper( // - node two becomes the lease holder for [c,e). Its timestamp cache does // not know about the read at 'd' which happened at the beginning. // - node two can illegally propose a write to 'd' at a lower timestamp. - // - // TODO(tschottdorf): why would this use r.store.Engine() and not the - // batch? We do the same thing for other usages of the state loader. sl := MakeStateLoader(rec) - leftLease, err := sl.LoadLease(ctx, rec.Engine()) + leftLease, err := sl.LoadLease(ctx, batch) if err != nil { return enginepb.MVCCStats{}, result.Result{}, errors.Wrap(err, "unable to load lease") } @@ -970,7 +967,7 @@ func splitTriggerHelper( } rightLease := leftLease rightLease.Replica = replica - gcThreshold, err := sl.LoadGCThreshold(ctx, rec.Engine()) + gcThreshold, err := sl.LoadGCThreshold(ctx, batch) if err != nil { return enginepb.MVCCStats{}, result.Result{}, errors.Wrap(err, "unable to load GCThreshold") } @@ -1001,7 +998,7 @@ func splitTriggerHelper( truncStateType = stateloader.TruncatedStateLegacyReplicated } - replicaVersion, err := sl.LoadVersion(ctx, rec.Engine()) + replicaVersion, err := sl.LoadVersion(ctx, batch) if err != nil { return enginepb.MVCCStats{}, result.Result{}, errors.Wrap(err, "unable to load GCThreshold") } diff --git a/pkg/kv/kvserver/batcheval/cmd_recompute_stats.go b/pkg/kv/kvserver/batcheval/cmd_recompute_stats.go index 997794349b8e..736d96a9d1c9 100644 --- a/pkg/kv/kvserver/batcheval/cmd_recompute_stats.go +++ b/pkg/kv/kvserver/batcheval/cmd_recompute_stats.go @@ -29,7 +29,7 @@ func init() { } func declareKeysRecomputeStats( - rs ImmutableRangeState, _ roachpb.Header, req roachpb.Request, latchSpans, _ *spanset.SpanSet, + rs ImmutableRangeState, _ roachpb.Header, _ roachpb.Request, latchSpans, _ *spanset.SpanSet, ) { // We don't declare any user key in the range. This is OK since all we're doing is computing a // stats delta, and applying this delta commutes with other operations on the same key space. @@ -53,7 +53,7 @@ func declareKeysRecomputeStats( // RecomputeStats recomputes the MVCCStats stored for this range and adjust them accordingly, // returning the MVCCStats delta obtained in the process. func RecomputeStats( - ctx context.Context, _ storage.Reader, cArgs CommandArgs, resp roachpb.Response, + ctx context.Context, reader storage.Reader, cArgs CommandArgs, resp roachpb.Response, ) (result.Result, error) { desc := cArgs.EvalCtx.Desc() args := cArgs.Args.(*roachpb.RecomputeStatsRequest) @@ -64,27 +64,16 @@ func RecomputeStats( args = nil // avoid accidental use below - // Open a snapshot from which we will read everything (including the - // MVCCStats). This is necessary because a batch does not provide us - // with a consistent view of the data -- reading from the batch, we - // could see skew between the stats recomputation and the MVCCStats - // we read from the range state if concurrent writes are inflight[1]. - // - // Note that in doing so, we also circumvent the assertions (present in both - // the EvalContext and the batch in some builds) which check that all reads - // were previously declared. See the comment in `declareKeysRecomputeStats` - // for details on this. - // - // [1]: see engine.TestBatchReadLaterWrite. - snap := cArgs.EvalCtx.Engine().NewSnapshot() - defer snap.Close() + // Disable the assertions which check that all reads were previously declared. + // See the comment in `declareKeysRecomputeStats` for details on this. + reader = spanset.DisableReaderAssertions(reader) - actualMS, err := rditer.ComputeStatsForRange(desc, snap, cArgs.Header.Timestamp.WallTime) + actualMS, err := rditer.ComputeStatsForRange(desc, reader, cArgs.Header.Timestamp.WallTime) if err != nil { return result.Result{}, err } - currentStats, err := MakeStateLoader(cArgs.EvalCtx).LoadMVCCStats(ctx, snap) + currentStats, err := MakeStateLoader(cArgs.EvalCtx).LoadMVCCStats(ctx, reader) if err != nil { return result.Result{}, err } diff --git a/pkg/kv/kvserver/batcheval/eval_context.go b/pkg/kv/kvserver/batcheval/eval_context.go index 333820f7668c..04dbd7e6f27e 100644 --- a/pkg/kv/kvserver/batcheval/eval_context.go +++ b/pkg/kv/kvserver/batcheval/eval_context.go @@ -23,7 +23,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/settings/cluster" - "github.com/cockroachdb/cockroach/pkg/storage" "github.com/cockroachdb/cockroach/pkg/storage/cloud" "github.com/cockroachdb/cockroach/pkg/storage/enginepb" "github.com/cockroachdb/cockroach/pkg/util/hlc" @@ -51,7 +50,6 @@ type EvalContext interface { ClusterSettings() *cluster.Settings EvalKnobs() kvserverbase.BatchEvalTestingKnobs - Engine() storage.Engine Clock() *hlc.Clock DB() *kv.DB AbortSpan() *abortspan.AbortSpan @@ -172,9 +170,6 @@ func (m *mockEvalCtxImpl) ClusterSettings() *cluster.Settings { func (m *mockEvalCtxImpl) EvalKnobs() kvserverbase.BatchEvalTestingKnobs { return kvserverbase.BatchEvalTestingKnobs{} } -func (m *mockEvalCtxImpl) Engine() storage.Engine { - panic("unimplemented") -} func (m *mockEvalCtxImpl) Clock() *hlc.Clock { return m.MockEvalCtx.Clock } diff --git a/pkg/kv/kvserver/replica_eval_context_span.go b/pkg/kv/kvserver/replica_eval_context_span.go index 0afcb0de2c15..5f9c1b225d95 100644 --- a/pkg/kv/kvserver/replica_eval_context_span.go +++ b/pkg/kv/kvserver/replica_eval_context_span.go @@ -25,7 +25,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/settings/cluster" - "github.com/cockroachdb/cockroach/pkg/storage" "github.com/cockroachdb/cockroach/pkg/storage/cloud" "github.com/cockroachdb/cockroach/pkg/storage/enginepb" "github.com/cockroachdb/cockroach/pkg/util/hlc" @@ -92,11 +91,6 @@ func (rec *SpanSetReplicaEvalContext) GetNodeLocality() roachpb.Locality { return rec.i.GetNodeLocality() } -// Engine returns the engine. -func (rec *SpanSetReplicaEvalContext) Engine() storage.Engine { - return rec.i.Engine() -} - // GetFirstIndex returns the first index. func (rec *SpanSetReplicaEvalContext) GetFirstIndex() (uint64, error) { return rec.i.GetFirstIndex() diff --git a/pkg/kv/kvserver/spanset/batch.go b/pkg/kv/kvserver/spanset/batch.go index 2ae3a19634d1..597ca71b2a3b 100644 --- a/pkg/kv/kvserver/spanset/batch.go +++ b/pkg/kv/kvserver/spanset/batch.go @@ -763,3 +763,16 @@ func NewBatchAt(b storage.Batch, spans *SpanSet, ts hlc.Timestamp) storage.Batch ts: ts, } } + +// DisableReaderAssertions unwraps any storage.Reader implementations that may +// assert access against a given SpanSet. +func DisableReaderAssertions(reader storage.Reader) storage.Reader { + switch v := reader.(type) { + case ReadWriter: + return DisableReaderAssertions(v.r) + case *spanSetBatch: + return DisableReaderAssertions(v.r) + default: + return reader + } +} From d4ea9e43aa7085fa728cb746bcff2a279ae4b942 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Thu, 1 Jul 2021 21:53:18 +0000 Subject: [PATCH 2/2] c-deps: remove protobuf Release note: none. --- .gitmodules | 3 --- BUILD.bazel | 1 - Makefile | 26 ++++---------------------- build/variables.mk | 3 --- c-deps/REPOSITORIES.bzl | 10 ---------- c-deps/protobuf | 1 - 6 files changed, 4 insertions(+), 40 deletions(-) delete mode 160000 c-deps/protobuf diff --git a/.gitmodules b/.gitmodules index 2b79db1d235c..e9d7c0e0e14e 100644 --- a/.gitmodules +++ b/.gitmodules @@ -4,9 +4,6 @@ [submodule "c-deps/jemalloc"] path = c-deps/jemalloc url = https://github.com/cockroachdb/jemalloc.git -[submodule "c-deps/protobuf"] - path = c-deps/protobuf - url = https://github.com/cockroachdb/protobuf.git [submodule "pkg/ui/yarn-vendor"] path = pkg/ui/yarn-vendor url = https://github.com/cockroachdb/yarn-vendored diff --git a/BUILD.bazel b/BUILD.bazel index f799ac84b482..e406acf3de46 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -72,7 +72,6 @@ load("@bazel_gazelle//:def.bzl", "gazelle") # # gazelle:exclude _bazel # gazelle:exclude c-deps/krb5 -# gazelle:exclude c-deps/protobuf # gazelle:exclude artifacts # gazelle:exclude vendor # gazelle:exclude .vendor.tmp.* diff --git a/Makefile b/Makefile index ebf19b42e303..78d8b4702fe0 100644 --- a/Makefile +++ b/Makefile @@ -487,7 +487,6 @@ endif C_DEPS_DIR := $(abspath c-deps) JEMALLOC_SRC_DIR := $(C_DEPS_DIR)/jemalloc -PROTOBUF_SRC_DIR := $(C_DEPS_DIR)/protobuf GEOS_SRC_DIR := $(C_DEPS_DIR)/geos PROJ_SRC_DIR := $(C_DEPS_DIR)/proj LIBEDIT_SRC_DIR := $(C_DEPS_DIR)/libedit @@ -512,16 +511,13 @@ BUILD_DIR := $(shell cygpath -m $(BUILD_DIR)) endif JEMALLOC_DIR := $(BUILD_DIR)/jemalloc -PROTOBUF_DIR := $(BUILD_DIR)/protobuf GEOS_DIR := $(BUILD_DIR)/geos PROJ_DIR := $(BUILD_DIR)/proj LIBEDIT_DIR := $(BUILD_DIR)/libedit LIBROACH_DIR := $(BUILD_DIR)/libroach$(if $(ENABLE_LIBROACH_ASSERTIONS),_assert) KRB5_DIR := $(BUILD_DIR)/krb5 -# Can't share with protobuf because protoc is always built for the host. LIBJEMALLOC := $(JEMALLOC_DIR)/lib/libjemalloc.a -LIBPROTOBUF := $(PROTOBUF_DIR)/libprotobuf.a LIBEDIT := $(LIBEDIT_DIR)/src/.libs/libedit.a LIBROACH := $(LIBROACH_DIR)/libroach.a LIBPROJ := $(PROJ_DIR)/lib/libproj$(if $(target-is-windows),_4_9).a @@ -543,8 +539,8 @@ C_LIBS_COMMON = \ $(if $(target-is-windows),,$(LIBEDIT)) \ $(LIBPROJ) $(LIBROACH) C_LIBS_SHORT = $(C_LIBS_COMMON) -C_LIBS_OSS = $(C_LIBS_COMMON) $(LIBPROTOBUF) -C_LIBS_CCL = $(C_LIBS_COMMON) $(LIBPROTOBUF) +C_LIBS_OSS = $(C_LIBS_COMMON) +C_LIBS_CCL = $(C_LIBS_COMMON) C_LIBS_DYNAMIC = $(LIBGEOS) # We only include krb5 on linux, non-musl builds. @@ -601,7 +597,7 @@ $(BASE_CGO_FLAGS_FILES): Makefile build/defs.mk.sig | bin/.submodules-initialize @echo 'package $(if $($(@D)-package),$($(@D)-package),$(notdir $(@D)))' >> $@ @echo >> $@ @echo '// #cgo CPPFLAGS: $(addprefix -I,$(JEMALLOC_DIR)/include $(KRB_CPPFLAGS))' >> $@ - @echo '// #cgo LDFLAGS: $(addprefix -L,$(PROTOBUF_DIR) $(JEMALLOC_DIR)/lib $(LIBEDIT_DIR)/src/.libs $(LIBROACH_DIR) $(KRB_DIR) $(PROJ_DIR)/lib)' >> $@ + @echo '// #cgo LDFLAGS: $(addprefix -L,$(JEMALLOC_DIR)/lib $(LIBEDIT_DIR)/src/.libs $(LIBROACH_DIR) $(KRB_DIR) $(PROJ_DIR)/lib)' >> $@ @echo 'import "C"' >> $@ vendor/github.com/knz/go-libedit/unix/zcgo_flags_extra.go: Makefile | bin/.submodules-initialized @@ -659,14 +655,6 @@ $(KRB5_DIR)/Makefile: $(C_DEPS_DIR)/krb5-rebuild $(KRB5_SRC_DIR)/src/configure @# We specify -fcommon to get around duplicate definition errors in recent gcc. cd $(KRB5_DIR) && env -u CXXFLAGS CFLAGS="-fcommon" $(KRB5_SRC_DIR)/src/configure $(xconfigure-flags) --enable-static --disable-shared -$(PROTOBUF_DIR)/Makefile: $(C_DEPS_DIR)/protobuf-rebuild | bin/.submodules-initialized - rm -rf $(PROTOBUF_DIR) - mkdir -p $(PROTOBUF_DIR) - @# NOTE: If you change the CMake flags below, bump the version in - @# $(C_DEPS_DIR)/protobuf-rebuild. See above for rationale. - cd $(PROTOBUF_DIR) && cmake $(xcmake-flags) -Dprotobuf_WITH_ZLIB=OFF -Dprotobuf_BUILD_TESTS=OFF $(PROTOBUF_SRC_DIR)/cmake \ - -DCMAKE_BUILD_TYPE=Release - $(GEOS_DIR)/Makefile: $(C_DEPS_DIR)/geos-rebuild | bin/.submodules-initialized rm -rf $(GEOS_DIR) mkdir -p $(GEOS_DIR) @@ -736,9 +724,6 @@ $(LIBEDIT_DIR)/Makefile: $(C_DEPS_DIR)/libedit-rebuild $(LIBEDIT_SRC_DIR)/config $(LIBJEMALLOC): $(JEMALLOC_DIR)/Makefile bin/uptodate .ALWAYS_REBUILD @uptodate $@ $(JEMALLOC_SRC_DIR) || $(MAKE) --no-print-directory -C $(JEMALLOC_DIR) build_lib_static -$(LIBPROTOBUF): $(PROTOBUF_DIR)/Makefile bin/uptodate .ALWAYS_REBUILD - @uptodate $@ $(PROTOBUF_SRC_DIR) || $(MAKE) --no-print-directory -C $(PROTOBUF_DIR) libprotobuf - ifdef is-cross-compile ifdef target-is-macos geos_require_install_name_tool := 1 @@ -789,10 +774,9 @@ $(LIBKRB5): $(KRB5_DIR)/Makefile bin/uptodate .ALWAYS_REBUILD @uptodate $@ $(KRB5_SRC_DIR)/src || $(MAKE) --no-print-directory -C $(KRB5_DIR) # Convenient names for maintainers. Not used by other targets in the Makefile. -.PHONY: libjemalloc libprotobuf libgeos libproj libroach libkrb5 +.PHONY: libjemalloc libgeos libproj libroach libkrb5 libedit: $(LIBEDIT) libjemalloc: $(LIBJEMALLOC) -libprotobuf: $(LIBPROTOBUF) libgeos: $(LIBGEOS) libproj: $(LIBPROJ) libroach: $(LIBROACH) @@ -1685,7 +1669,6 @@ c-deps-fmt: .PHONY: clean-c-deps clean-c-deps: rm -rf $(JEMALLOC_DIR) - rm -rf $(PROTOBUF_DIR) rm -rf $(GEOS_DIR) rm -rf $(PROJ_DIR) rm -rf $(LIBROACH_DIR) @@ -1694,7 +1677,6 @@ clean-c-deps: .PHONY: unsafe-clean-c-deps unsafe-clean-c-deps: git -C $(JEMALLOC_SRC_DIR) clean -dxf - git -C $(PROTOBUF_SRC_DIR) clean -dxf git -C $(GEOS_SRC_DIR) clean -dxf git -C $(PROJ_SRC_DIR) clean -dxf git -C $(LIBROACH_SRC_DIR) clean -dxf diff --git a/build/variables.mk b/build/variables.mk index 0c051a483714..0fe0471fc05d 100644 --- a/build/variables.mk +++ b/build/variables.mk @@ -94,7 +94,6 @@ define VALID_VARS LIBGEOS LIBJEMALLOC LIBPROJ - LIBPROTOBUF LIBROACH LIBROACH_DIR LIBROACH_SRC_DIR @@ -117,8 +116,6 @@ define VALID_VARS PROJ_DIR PROJ_SRC_DIR PROMETHEUS_PATH - PROTOBUF_DIR - PROTOBUF_SRC_DIR PROTOBUF_TARGETS PROTO_MAPPINGS RACETIMEOUT diff --git a/c-deps/REPOSITORIES.bzl b/c-deps/REPOSITORIES.bzl index 9319dedc4904..f249a18b2354 100644 --- a/c-deps/REPOSITORIES.bzl +++ b/c-deps/REPOSITORIES.bzl @@ -11,11 +11,6 @@ BUILD_ALL_CONTENT = """filegroup(name = "all", srcs = glob(["**"]), visibility = # building those libraries require certain checked out repositories being # placed relative to the source tree of the library itself. -# For c-deps/protobuf, we elide a checked in generated file. Already generated -# files are read-only in the bazel sandbox, so bazel is unable to regenerate -# the same files, which the build process requires it to do so. -BUILD_PROTOBUF_CONTENT = """filegroup(name = "all", srcs = glob(["**"], exclude=["src/google/protobuf/compiler/js/well_known_types_embed.cc"]), visibility = ["//visibility:public"])""" - # This is essentially the same above, we elide a generated file to avoid # permission issues when building jemalloc within the bazel sandbox. BUILD_JEMALLOC_CONTENT = """filegroup(name = "all", srcs = glob(["**"], exclude=["configure"]), visibility = ["//visibility:public"])""" @@ -37,11 +32,6 @@ def c_deps(): path = "c-deps/geos", build_file_content = BUILD_ALL_CONTENT, ) - native.new_local_repository( - name = "protobuf", - path = "c-deps/protobuf", - build_file_content = BUILD_PROTOBUF_CONTENT, - ) native.new_local_repository( name = "jemalloc", path = "c-deps/jemalloc", diff --git a/c-deps/protobuf b/c-deps/protobuf deleted file mode 160000 index e809d75ecb57..000000000000 --- a/c-deps/protobuf +++ /dev/null @@ -1 +0,0 @@ -Subproject commit e809d75ecb5770fdc531081eef306b3e672bcdd2