diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 055d45036da3..2f6720517a39 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -6,19 +6,28 @@ # - every file under ./pkg has to have at least one owner, and each owner must # be present in TEAMS.yaml (either as a map key or an alias). # - you can opt out of GitHub-requested code review assignments (while -# maintaining team ownership) by suffixing the handle with `-noreview`. -# (This will essentially make it an unknown team to GitHub, but our internal -# tooling continues to recognize the original team). +# maintaining team ownership) prefixing the line with `#!`. +# (This will hide the line from GitHub, but our internal tooling continues to +# parse it). +# - there is a special team @cockroachdb/unowned (only to be used with #! prefix as +# to not confuse Github) for the rare situations in which a file has no canonical owner. +# Please use this sparingly. +# +# TODO(test-eng): it would be good to lint that following a `#!` marker all mentioned +# teams match @cockroachdb/{unowned,*-noreview}. # # Remember, *the last rule to match wins*, and you need a trailing slash to get # recursive ownership of a directory. # +# When you send a PR to update this file, please look at the "Files" tab and +# fix any errors Github reports. +# # [1]: https://github.com/blog/2392-introducing-code-owners # [2]: https://help.github.com/articles/about-codeowners/ # [3]: pkg/internal/codeowners /.github/ @cockroachdb/dev-inf -/.github/CODEOWNERS @cockroachdb/unowned +#!/.github/CODEOWNERS @cockroachdb/unowned /build/ @cockroachdb/dev-inf @@ -27,12 +36,12 @@ /Makefile @cockroachdb/dev-inf -/pkg/sql/ @cockroachdb/sql-queries-noreview +#!/pkg/sql/ @cockroachdb/sql-queries-noreview /pkg/sql/inverted/ @cockroachdb/sql-queries /pkg/sql/opt/ @cockroachdb/sql-queries /pkg/sql/opt_*.go @cockroachdb/sql-queries -/pkg/sql/opt/exec/execbuilder/testdata/ @cockroachdb/sql-queries-noreview +#!/pkg/sql/opt/exec/execbuilder/testdata/ @cockroachdb/sql-queries-noreview /pkg/sql/plan_opt*.go @cockroachdb/sql-queries /pkg/sql/querycache/ @cockroachdb/sql-queries /pkg/sql/span/ @cockroachdb/sql-queries @@ -41,8 +50,8 @@ /pkg/sql/col* @cockroachdb/sql-queries /pkg/sql/distsql*.go @cockroachdb/sql-queries /pkg/sql/exec* @cockroachdb/sql-queries -/pkg/sql/exec_log*.go @cockroachdb/sql-queries-noreview -/pkg/sql/exec_util*.go @cockroachdb/sql-queries-noreview +#!/pkg/sql/exec_log*.go @cockroachdb/sql-queries-noreview +#!/pkg/sql/exec_util*.go @cockroachdb/sql-queries-noreview /pkg/sql/flowinfra/ @cockroachdb/sql-queries /pkg/sql/physicalplan/ @cockroachdb/sql-queries /pkg/sql/row* @cockroachdb/sql-queries @@ -95,7 +104,7 @@ /pkg/cli/ @cockroachdb/cli-prs # last-rule-wins so bulk i/o takes userfile.go even though cli-prs takes pkg/cli /pkg/cli/userfile.go @cockroachdb/disaster-recovery -/pkg/cli/auth.go @cockroachdb/unowned @cockroachdb/prodsec @cockroachdb/cli-prs +/pkg/cli/auth.go @cockroachdb/prodsec @cockroachdb/cli-prs /pkg/cli/cert*.go @cockroachdb/cli-prs @cockroachdb/prodsec /pkg/cli/demo*.go @cockroachdb/sql-sessions @cockroachdb/server-prs @cockroachdb/cli-prs /pkg/cli/democluster @cockroachdb/sql-sessions @cockroachdb/server-prs @cockroachdb/cli-prs @@ -112,19 +121,19 @@ /pkg/cli/mt_proxy.go @cockroachdb/sqlproxy-prs @cockroachdb/server-prs /pkg/cli/mt_start_sql.go @cockroachdb/sqlproxy-prs @cockroachdb/server-prs /pkg/cli/mt_test_directory.go @cockroachdb/sqlproxy-prs @cockroachdb/server-prs -/pkg/cli/connect*.go @cockroachdb/unowned @cockroachdb/cli-prs @cockroachdb/prodsec -/pkg/cli/init.go @cockroachdb/unowned @cockroachdb/cli-prs +/pkg/cli/connect*.go @cockroachdb/cli-prs @cockroachdb/prodsec +/pkg/cli/init.go @cockroachdb/cli-prs /pkg/cli/log*.go @cockroachdb/obs-inf-prs @cockroachdb/cli-prs /pkg/cli/debug_logconfig.go @cockroachdb/obs-inf-prs @cockroachdb/cli-prs /pkg/cli/debug_merg_logs*.go @cockroachdb/obs-inf-prs @cockroachdb/cli-prs /pkg/server/ @cockroachdb/cli-prs -/pkg/server/addjoin*.go @cockroachdb/unowned @cockroachdb/server-prs @cockroachdb/prodsec +/pkg/server/addjoin*.go @cockroachdb/server-prs @cockroachdb/prodsec /pkg/server/admin*.go @cockroachdb/obs-inf-prs @cockroachdb/server-prs /pkg/server/api_v2*.go @cockroachdb/obs-inf-prs @cockroachdb/server-prs -/pkg/server/api_v2_auth*.go @cockroachdb/unowned @cockroachdb/obs-inf-prs @cockroachdb/server-prs @cockroachdb/prodsec -/pkg/server/authentication*.go @cockroachdb/unowned @cockroachdb/server-prs @cockroachdb/prodsec -/pkg/server/auto_tls_init*go @cockroachdb/unowned @cockroachdb/server-prs @cockroachdb/prodsec +/pkg/server/api_v2_auth*.go @cockroachdb/obs-inf-prs @cockroachdb/server-prs @cockroachdb/prodsec +/pkg/server/authentication*.go @cockroachdb/server-prs @cockroachdb/prodsec +/pkg/server/auto_tls_init*go @cockroachdb/server-prs @cockroachdb/prodsec /pkg/server/clock_monotonicity.go @cockroachdb/kv-prs /pkg/server/combined_statement_stats*.go @cockroachdb/sql-observability @cockroachdb/obs-inf-prs /pkg/server/decommission*.go @cockroachdb/kv-prs @cockroachdb/server-prs @@ -134,7 +143,7 @@ /pkg/server/heapprofiler/ @cockroachdb/obs-inf-prs @cockroachdb/server-prs /pkg/server/import_ts*.go @cockroachdb/obs-inf-prs @cockroachdb/server-prs @cockroachdb/kv-prs /pkg/server/init*.go @cockroachdb/kv-prs @cockroachdb/server-prs -/pkg/server/init_handshake.go @cockroachdb/unowned @cockroachdb/server-prs @cockroachdb/prodsec +/pkg/server/init_handshake.go @cockroachdb/server-prs @cockroachdb/prodsec /pkg/server/loss_of_quorum*.go @cockroachdb/kv-prs /pkg/server/node_http*.go @cockroachdb/obs-inf-prs @cockroachdb/server-prs /pkg/server/node_tenant*go @cockroachdb/obs-inf-prs @cockroachdb/multi-tenant @cockroachdb/server-prs @@ -143,7 +152,7 @@ /pkg/server/server_http*.go @cockroachdb/obs-inf-prs @cockroachdb/server-prs /pkg/server/server_import_ts*.go @cockroachdb/obs-inf-prs @cockroachdb/kv-prs /pkg/server/serverpb/ @cockroachdb/obs-inf-prs @cockroachdb/server-prs -/pkg/server/serverpb/authentication* @cockroachdb/unowned @cockroachdb/obs-inf-prs @cockroachdb/prodsec @cockroachdb/server-prs +/pkg/server/serverpb/authentication* @cockroachdb/obs-inf-prs @cockroachdb/prodsec @cockroachdb/server-prs /pkg/server/serverpb/index_reco* @cockroachdb/sql-observability @cockroachdb/obs-inf-prs /pkg/server/serverrules/ @cockroachdb/obs-inf-prs @cockroachdb/server-prs /pkg/server/settingswatcher/ @cockroachdb/multi-tenant @cockroachdb/server-prs @@ -153,7 +162,7 @@ /pkg/server/status/ @cockroachdb/obs-inf-prs @cockroachdb/server-prs /pkg/server/systemconfigwatcher/ @cockroachdb/kv-prs @cockroachdb/multi-tenant /pkg/server/tenant*.go @cockroachdb/obs-inf-prs @cockroachdb/multi-tenant @cockroachdb/server-prs -/pkg/server/tenantsettingswatcher/ @cockroachdb/unowned @cockroachdb/multi-tenant +/pkg/server/tenantsettingswatcher/ @cockroachdb/multi-tenant /pkg/server/testserver*.go @cockroachdb/test-eng @cockroachdb/server-prs /pkg/server/tracedumper/ @cockroachdb/obs-inf-prs @cockroachdb/server-prs /pkg/server/user*.go @cockroachdb/obs-inf-prs @cockroachdb/server-prs @cockroachdb/prodsec @@ -161,7 +170,9 @@ /pkg/ccl/jobsccl/ @cockroachdb/jobs-prs /pkg/ccl/changefeedccl/ @cockroachdb/cdc-prs -/pkg/ccl/streamingccl/ @cockroachdb/tenant-streaming +# TODO(dt): either add the cockroach repository to this team or remove this team from +# CODEOWNERS. +#!/pkg/ccl/streamingccl/ @cockroachdb/tenant-streaming /pkg/ccl/backupccl/ @cockroachdb/disaster-recovery /pkg/ccl/backupccl/*_job.go @cockroachdb/disaster-recovery @cockroachdb/jobs-prs @@ -170,19 +181,80 @@ /pkg/cloud/ @cockroachdb/disaster-recovery /pkg/sql/distsql_plan_csv.go @cockroachdb/disaster-recovery -/pkg/geo/ @cockroachdb/geospatial +/pkg/geo/ @cockroachdb/spatial + +# The KV team generally owns ./pkg/kv/... but not all of it. By convention, +# inside of the /pkg/kv tree, we list out rules for each subdirectory, i.e. when +# a new directory is created CODEOWNERS should mandate a new line below. This +# serves as a lint that ownership is properly considered at creation time. +/pkg/kv/*.* @cockroachdb/kv-prs +/pkg/kv/bulk/ @cockroachdb/disaster-recovery +/pkg/kv/kvbase/ @cockroachdb/kv-prs +/pkg/kv/kvclient/ @cockroachdb/kv-prs +/pkg/kv/kvclient/kvcoord/*rangefeed* @cockroachdb/repl-prs +/pkg/kv/kvclient/kvstreamer @cockroachdb/sql-queries +/pkg/kv/kvclient/rangefeed/ @cockroachdb/repl-prs +/pkg/kv/kvnemesis/ @cockroachdb/kv-prs +/pkg/kv/kvprober/ @cockroachdb/kv-prs +# Same subdirectory rule as above for `/pkg/kv` +/pkg/kv/kvserver/*.* @cockroachdb/kv-prs +/pkg/kv/kvserver/*circuit*.go @cockroachdb/repl-prs +/pkg/kv/kvserver/*closed*ts*.go @cockroachdb/repl-prs +/pkg/kv/kvserver/*_app*.go @cockroachdb/repl-prs +/pkg/kv/kvserver/*closed_timestamp*.go @cockroachdb/repl-prs +/pkg/kv/kvserver/*consistency*.go @cockroachdb/repl-prs +/pkg/kv/kvserver/*probe*.go @cockroachdb/repl-prs +/pkg/kv/kvserver/*proposal*.go @cockroachdb/repl-prs +/pkg/kv/kvserver/*raft*.go @cockroachdb/repl-prs +/pkg/kv/kvserver/*raft*/ @cockroachdb/repl-prs +/pkg/kv/kvserver/*rangefeed*.go @cockroachdb/repl-prs +/pkg/kv/kvserver/*sideload*.go @cockroachdb/repl-prs +/pkg/kv/kvserver/abortspan/ @cockroachdb/kv-prs +/pkg/kv/kvserver/allocator/ @cockroachdb/kv-prs +/pkg/kv/kvserver/apply/ @cockroachdb/repl-prs +/pkg/kv/kvserver/asim/ @cockroachdb/kv-prs +/pkg/kv/kvserver/batcheval/ @cockroachdb/kv-prs +/pkg/kv/kvserver/closedts/ @cockroachdb/repl-prs +/pkg/kv/kvserver/concurrency/ @cockroachdb/kv-prs +/pkg/kv/kvserver/constraint/ @cockroachdb/kv-prs +/pkg/kv/kvserver/diskmap/ @cockroachdb/kv-prs +/pkg/kv/kvserver/gc/ @cockroachdb/kv-prs +/pkg/kv/kvserver/idalloc/ @cockroachdb/kv-prs +/pkg/kv/kvserver/intentresolver/ @cockroachdb/kv-prs +/pkg/kv/kvserver/kvadmission/ @cockroachdb/kv-prs +/pkg/kv/kvserver/kvserverbase/ @cockroachdb/kv-prs +/pkg/kv/kvserver/kvserverpb/ @cockroachdb/kv-prs +/pkg/kv/kvserver/liveness/ @cockroachdb/kv-prs +/pkg/kv/kvserver/logstore/ @cockroachdb/repl-prs +/pkg/kv/kvserver/loqrecovery/ @cockroachdb/repl-prs +/pkg/kv/kvserver/multiqueue/ @cockroachdb/kv-prs +/pkg/kv/kvserver/protectedts/ @cockroachdb/repl-prs +/pkg/kv/kvserver/rangefeed/ @cockroachdb/repl-prs +/pkg/kv/kvserver/rangelog/ @cockroachdb/kv-prs +/pkg/kv/kvserver/rditer/ @cockroachdb/repl-prs +/pkg/kv/kvserver/readsummary/ @cockroachdb/kv-prs +/pkg/kv/kvserver/replicastats/ @cockroachdb/kv-prs +/pkg/kv/kvserver/reports/ @cockroachdb/kv-prs +/pkg/kv/kvserver/spanlatch/ @cockroachdb/kv-prs +/pkg/kv/kvserver/spanset/ @cockroachdb/kv-prs +/pkg/kv/kvserver/split/ @cockroachdb/kv-prs +/pkg/kv/kvserver/stateloader/ @cockroachdb/kv-prs +/pkg/kv/kvserver/tenantrate/ @cockroachdb/kv-prs +#!/pkg/kv/kvserver/testdata/ @cockroachdb/kv-prs-noreview +/pkg/kv/kvserver/tscache/ @cockroachdb/kv-prs +/pkg/kv/kvserver/txnrecovery/ @cockroachdb/kv-prs +/pkg/kv/kvserver/txnwait/ @cockroachdb/kv-prs +/pkg/kv/kvserver/uncertainty/ @cockroachdb/kv-prs -/pkg/kv/ @cockroachdb/kv-prs /pkg/ccl/spanconfigccl/ @cockroachdb/kv-prs -/pkg/kv/kvclient/kvstreamer @cockroachdb/sql-queries /pkg/ccl/storageccl/engineccl @cockroachdb/storage /pkg/storage/ @cockroachdb/storage -/pkg/ui/ @cockroachdb/cluster-ui-prs -/pkg/ui/embedded.go @cockroachdb/cluster-ui-prs -/pkg/ui/src/js/protos.d.ts @cockroachdb/cluster-ui-prs -/pkg/ui/src/js/protos.js @cockroachdb/cluster-ui-prs +/pkg/ui/ @cockroachdb/admin-ui-prs +/pkg/ui/embedded.go @cockroachdb/admin-ui-prs +/pkg/ui/src/js/protos.d.ts @cockroachdb/admin-ui-prs +/pkg/ui/src/js/protos.js @cockroachdb/admin-ui-prs /docs/generated/http/ @cockroachdb/http-api-prs @cockroachdb/server-prs /pkg/cmd/docgen/http.go @cockroachdb/http-api-prs @cockroachdb/server-prs @@ -190,12 +262,12 @@ /pkg/ccl/sqlproxyccl/ @cockroachdb/sqlproxy-prs @cockroachdb/server-prs /pkg/gen/ @cockroachdb/dev-inf -/pkg/gen/*.bzl @cockroachdb/dev-inf-noreview +#!/pkg/gen/*.bzl @cockroachdb/dev-inf-noreview /pkg/gen/gen.bzl @cockroachdb/dev-inf /pkg/acceptance/ @cockroachdb/sql-sessions -/pkg/base/ @cockroachdb/unowned @cockroachdb/kv-prs @cockroachdb/server-prs -/pkg/bench/ @cockroachdb/sql-queries-noreview +/pkg/base/ @cockroachdb/kv-prs @cockroachdb/server-prs +#!/pkg/bench/ @cockroachdb/sql-queries-noreview /pkg/bench/rttanalysis @cockroachdb/sql-schema /pkg/blobs/ @cockroachdb/disaster-recovery /pkg/build/ @cockroachdb/dev-inf @@ -203,18 +275,18 @@ /pkg/ccl/buildccl/ @cockroachdb/dev-inf /pkg/ccl/cliccl/ @cockroachdb/cli-prs /pkg/ccl/cmdccl/stub-schema-registry/ @cockroachdb/cdc-prs -/pkg/ccl/gssapiccl/ @cockroachdb/unowned +#!/pkg/ccl/gssapiccl/ @cockroachdb/unowned /pkg/ccl/jwtauthccl/ @cockroachdb/cloud-identity -/pkg/ccl/kvccl/ @cockroachdb/kv-noreview +#!/pkg/ccl/kvccl/ @cockroachdb/kv-noreview /pkg/ccl/kvccl/kvtenantccl/ @cockroachdb/multi-tenant -/pkg/ccl/upgradeccl/ @cockroachdb/unowned -/pkg/ccl/logictestccl/ @cockroachdb/sql-queries-noreview -/pkg/ccl/sqlitelogictestccl/ @cockroachdb/sql-queries-noreview +#!/pkg/ccl/upgradeccl/ @cockroachdb/unowned +#!/pkg/ccl/logictestccl/ @cockroachdb/sql-queries-noreview +#!/pkg/ccl/sqlitelogictestccl/ @cockroachdb/sql-queries-noreview /pkg/ccl/multiregionccl/ @cockroachdb/multiregion /pkg/ccl/multitenantccl/ @cockroachdb/multi-tenant -/pkg/ccl/oidcccl/ @cockroachdb/unowned +#!/pkg/ccl/oidcccl/ @cockroachdb/unowned /pkg/ccl/partitionccl/ @cockroachdb/sql-schema @cockroachdb/multiregion -/pkg/ccl/serverccl/ @cockroachdb/unowned @cockroachdb/server-prs +/pkg/ccl/serverccl/ @cockroachdb/server-prs /pkg/ccl/serverccl/server_sql* @cockroachdb/multi-tenant @cockroachdb/server-prs /pkg/ccl/serverccl/tenant_* @cockroachdb/multi-tenant @cockroachdb/server-prs /pkg/ccl/serverccl/statusccl @cockroachdb/sql-observability @cockroachdb/multi-tenant @@ -222,11 +294,11 @@ /pkg/ccl/testccl/authccl/ @cockroachdb/cloud-identity /pkg/ccl/testccl/sqlccl/ @cockroachdb/sql-queries /pkg/ccl/testccl/workload/schemachange/ @cockroachdb/sql-schema -/pkg/ccl/testutilsccl/ @cockroachdb/test-eng-noreview -/pkg/ccl/utilccl/ @cockroachdb/unowned @cockroachdb/server-prs -/pkg/ccl/workloadccl/ @cockroachdb/sql-sessions-noreview @cockroachdb/test-eng +#!/pkg/ccl/testutilsccl/ @cockroachdb/test-eng-noreview +/pkg/ccl/utilccl/ @cockroachdb/server-prs +/pkg/ccl/workloadccl/ @cockroachdb/test-eng #! @cockroachdb/sql-sessions-noreview /pkg/ccl/benchccl/rttanalysisccl/ @cockroachdb/sql-schema -/pkg/clusterversion/ @cockroachdb/kv-prs-noreview +#!/pkg/clusterversion/ @cockroachdb/kv-prs-noreview /pkg/cmd/allocsim/ @cockroachdb/kv-prs /pkg/cmd/bazci/ @cockroachdb/dev-inf /pkg/cmd/cloudupload/ @cockroachdb/dev-inf @@ -241,17 +313,17 @@ /pkg/cmd/compile-build/ @cockroachdb/dev-inf /pkg/cmd/cr2pg/ @cockroachdb/sql-sessions /pkg/cmd/dev/ @cockroachdb/dev-inf -/pkg/cmd/docgen/ @cockroachdb/docs +#!/pkg/cmd/docgen/ @cockroachdb/docs-infra-prs /pkg/cmd/docs-issue-generation/ @cockroachdb/dev-inf /pkg/cmd/fuzz/ @cockroachdb/test-eng /pkg/cmd/generate-binary/ @cockroachdb/sql-sessions /pkg/cmd/generate-distdir/ @cockroachdb/dev-inf /pkg/cmd/generate-logictest/ @cockroachdb/dev-inf /pkg/cmd/generate-metadata-tables/ @cockroachdb/sql-sessions -/pkg/cmd/generate-spatial-ref-sys/ @cockroachdb/geospatial +/pkg/cmd/generate-spatial-ref-sys/ @cockroachdb/spatial /pkg/cmd/generate-bazel-extra/ @cockroachdb/dev-inf /pkg/cmd/generate-staticcheck/ @cockroachdb/dev-inf -/pkg/cmd/geoviz/ @cockroachdb/geospatial +/pkg/cmd/geoviz/ @cockroachdb/spatial /pkg/cmd/github-post/ @cockroachdb/test-eng /pkg/cmd/github-pull-request-make/ @cockroachdb/dev-inf /pkg/cmd/gossipsim/ @cockroachdb/kv-prs @@ -276,7 +348,7 @@ # is the Owner for the roachtest, but until we unify these # two concepts of ownership we don't want to ping test-eng # on each test change. -/pkg/cmd/roachtest/tests @cockroachdb/test-eng-noreview +#!/pkg/cmd/roachtest/tests @cockroachdb/test-eng-noreview /pkg/cmd/roachvet/ @cockroachdb/dev-inf /pkg/cmd/skip-test/ @cockroachdb/test-eng /pkg/cmd/skiperrs/ @cockroachdb/sql-sessions @@ -287,16 +359,19 @@ /pkg/cmd/teamcity-trigger/ @cockroachdb/dev-inf /pkg/cmd/testfilter/ @cockroachdb/test-eng /pkg/cmd/uptodate/ @cockroachdb/dev-inf -/pkg/cmd/urlcheck/ @cockroachdb/docs +#!/pkg/cmd/urlcheck/ @cockroachdb/docs-infra-prs /pkg/cmd/whoownsit/ @cockroachdb/test-eng -/pkg/cmd/workload/ @cockroachdb/sql-sessions-noreview @cockroachdb/test-eng -/pkg/cmd/wraprules/ @cockroachdb/obs-inf-prs-noreview -/pkg/cmd/zerosum/ @cockroachdb/kv-noreview +/pkg/cmd/workload/ @cockroachdb/test-eng #! @cockroachdb/sql-sessions-noreview +#!/pkg/cmd/wraprules/ @cockroachdb/obs-inf-prs-noreview +#!/pkg/cmd/zerosum/ @cockroachdb/kv-noreview /pkg/col/ @cockroachdb/sql-queries /pkg/compose/ @cockroachdb/sql-sessions /pkg/config/ @cockroachdb/kv-prs @cockroachdb/server-prs -/pkg/docs/ @cockroachdb/docs -/pkg/featureflag/ @cockroachdb/cli-prs-noreview +# TODO(nickvigilante): add the cockroach repo to the docs-infra-prs team so that +# Github stops complaining. Then remove the #! prefix here and on the other lines +# that mention this team. +#!/pkg/docs/ @cockroachdb/docs-infra-prs +#!/pkg/featureflag/ @cockroachdb/cli-prs-noreview /pkg/gossip/ @cockroachdb/kv-prs /pkg/internal/client/requestbatcher/ @cockroachdb/kv-prs /pkg/internal/codeowners/ @cockroachdb/test-eng @@ -308,15 +383,15 @@ /pkg/keys/ @cockroachdb/kv-prs /pkg/keysbase/ @cockroachdb/kv-prs # Don't ping KV on updates to reserved descriptor IDs and such. -/pkg/keys/constants.go @cockroachdb/kv-prs-noreview -/pkg/upgrade/ @cockroachdb/kv-prs-noreview @cockroachdb/sql-schema +#!/pkg/keys/constants.go @cockroachdb/kv-prs-noreview +/pkg/upgrade/ @cockroachdb/sql-schema /pkg/multitenant/ @cockroachdb/multi-tenant /pkg/release/ @cockroachdb/dev-inf /pkg/roachpb/.gitattributes @cockroachdb/dev-inf /pkg/roachpb/ambiguous_* @cockroachdb/kv-prs /pkg/roachpb/api* @cockroachdb/kv-prs /pkg/roachpb/batch* @cockroachdb/kv-prs -/pkg/roachpb/BUILD.bazel @cockroachdb/kv-prs-noreview +#!/pkg/roachpb/BUILD.bazel @cockroachdb/kv-prs-noreview /pkg/roachpb/data* @cockroachdb/kv-prs /pkg/roachpb/dep_test.go @cockroachdb/dev-inf /pkg/roachpb/error* @cockroachdb/kv-prs @@ -326,7 +401,7 @@ /pkg/roachpb/index* @cockroachdb/sql-observability /pkg/roachpb/internal* @cockroachdb/kv-prs /pkg/roachpb/io-formats* @cockroachdb/disaster-recovery -/pkg/roachpb/main_test.go @cockroachdb/kv-prs-noreview +#!/pkg/roachpb/main_test.go @cockroachdb/kv-prs-noreview /pkg/roachpb/merge_spans* @cockroachdb/kv-prs /pkg/roachpb/metadata* @cockroachdb/kv-prs /pkg/roachpb/method* @cockroachdb/kv-prs @@ -337,23 +412,23 @@ /pkg/roachpb/tenant* @cockroachdb/kv-prs /pkg/roachpb/testdata/ambi* @cockroachdb/kv-prs /pkg/roachpb/testdata/repl* @cockroachdb/kv-prs -/pkg/roachpb/version* @cockroachdb/unowned +#!/pkg/roachpb/version* @cockroachdb/unowned /pkg/roachprod/ @cockroachdb/test-eng /pkg/rpc/ @cockroachdb/kv-prs -/pkg/rpc/auth.go @cockroachdb/unowned @cockroachdb/server-prs @cockroachdb/kv-prs @cockroachdb/prodsec +/pkg/rpc/auth.go @cockroachdb/server-prs @cockroachdb/kv-prs @cockroachdb/prodsec /pkg/scheduledjobs/ @cockroachdb/jobs-prs -/pkg/security/ @cockroachdb/unowned @cockroachdb/server-prs @cockroachdb/prodsec -/pkg/security/clientsecopts/ @cockroachdb/unowned @cockroachdb/server-prs @cockroachdb/sql-sessions @cockroachdb/prodsec -/pkg/settings/ @cockroachdb/unowned +/pkg/security/ @cockroachdb/server-prs @cockroachdb/prodsec +/pkg/security/clientsecopts/ @cockroachdb/server-prs @cockroachdb/sql-sessions @cockroachdb/prodsec +#!/pkg/settings/ @cockroachdb/unowned /pkg/spanconfig/ @cockroachdb/kv-prs /pkg/repstream/ @cockroachdb/disaster-recovery -/pkg/testutils/ @cockroachdb/test-eng-noreview +#!/pkg/testutils/ @cockroachdb/test-eng-noreview /pkg/testutils/reduce/ @cockroachdb/sql-queries /pkg/testutils/sqlutils/ @cockroachdb/sql-queries /pkg/testutils/jobutils/ @cockroachdb/jobs-prs /pkg/ts/ @cockroachdb/kv-prs /pkg/ts/catalog/ @cockroachdb/obs-inf-prs -/pkg/util/ @cockroachdb/unowned +#!/pkg/util/ @cockroachdb/unowned /pkg/util/log/ @cockroachdb/obs-inf-prs /pkg/util/addr/ @cockroachdb/cli-prs @cockroachdb/obs-inf-prs /pkg/util/metric/ @cockroachdb/obs-inf-prs @@ -362,17 +437,21 @@ /pkg/util/admission/ @cockroachdb/admission-control /pkg/util/schedulerlatency/ @cockroachdb/admission-control /pkg/util/tracing @cockroachdb/obs-inf-prs -/pkg/workload/ @cockroachdb/sql-sessions-noreview @cockroachdb/test-eng +/pkg/workload/ @cockroachdb/test-eng #! @cockroachdb/sql-sessions-noreview /pkg/obs/ @cockroachdb/obs-inf-prs /pkg/obsservice/ @cockroachdb/obs-inf-prs # Own all bazel files to dev-inf, but don't request reviews for them # as they are mostly - but not only - generated code that changes with # changes to the Go code in the package. -**/BUILD.bazel @cockroachdb/dev-inf-noreview -# Avoid mass pings when updating proto tooling. +#!**/BUILD.bazel @cockroachdb/dev-inf-noreview + +# Own the generated proto files to someone. They're not +# checked in, but since our owners tooling isn't aware +# of that we still want this rule to pass lints locally. +# # For some reason, **/*.pb.go does not work (in the # sense that ./pkg/cmd/whoownsit will not match this # pattern to any files). -**.pb.go @cockroachdb/unowned -**.pb.gw.go @cockroachdb/unowned +#!**.pb.go @cockroachdb/unowned +#!**.pb.gw.go @cockroachdb/unowned diff --git a/TEAMS.yaml b/TEAMS.yaml index d20b863c51a8..1121d439ef92 100644 --- a/TEAMS.yaml +++ b/TEAMS.yaml @@ -19,6 +19,8 @@ cockroachdb/docs: triage_column_id: 3971225 + aliases: + cockroachdb/docs-infra-prs: other cockroachdb/sql-sessions: aliases: cockroachdb/sql-syntax-prs: other @@ -45,7 +47,7 @@ cockroachdb/replication: aliases: cockroachdb/repl-prs: other label: T-kv-replication -cockroachdb/geospatial: +cockroachdb/spatial: triage_column_id: 9487269 cockroachdb/dev-inf: triage_column_id: 10210759 @@ -73,9 +75,9 @@ cockroachdb/server: cockroachdb/server-prs: other cockroachdb/http-api-prs: other triage_column_id: 2521812 -cockroachdb/cluster-ui: +cockroachdb/admin-ui: aliases: - cockroachdb/cluster-ui-prs: other + cockroachdb/admin-ui-prs: other triage_column_id: 6598672 cockroachdb/obs-inf-prs: triage_column_id: 14196277 diff --git a/docs/design.md b/docs/design.md index 8b962a5b76f5..9b2a3c4007d6 100644 --- a/docs/design.md +++ b/docs/design.md @@ -201,11 +201,11 @@ implementation of SSI still requires no locking, but will end up aborting more transactions. Cockroach’s SI and SSI implementations prevent starvation scenarios even for arbitrarily long transactions. -See the [Cahill paper](https://drive.google.com/file/d/0B9GCVTp_FHJIcEVyZVdDWEpYYXVVbFVDWElrYUV0NHFhU2Fv/edit?usp=sharing) +See the [Cahill paper](https://ses.library.usyd.edu.au/bitstream/handle/2123/5353/michael-cahill-2009-thesis.pdf) for one possible implementation of SSI. This is another [great paper](http://cs.yale.edu/homes/thomson/publications/calvin-sigmod12.pdf). For a discussion of SSI implemented by preventing read-write conflicts (in contrast to detecting them, called write-snapshot isolation), see -the [Yabandeh paper](https://drive.google.com/file/d/0B9GCVTp_FHJIMjJ2U2t6aGpHLTFUVHFnMTRUbnBwc2pLa1RN/edit?usp=sharing), +the [Yabandeh paper](https://courses.cs.washington.edu/courses/cse444/10sp/544M/READING-LIST/fekete-sigmod2008.pdf), which is the source of much inspiration for Cockroach’s SSI. Both SI and SSI require that the outcome of reads must be preserved, i.e. diff --git a/pkg/cmd/bazci/bazci.go b/pkg/cmd/bazci/bazci.go index 73c19f0f4ce6..2e1f0027ec4f 100644 --- a/pkg/cmd/bazci/bazci.go +++ b/pkg/cmd/bazci/bazci.go @@ -216,6 +216,9 @@ func (s *monitorBuildServer) handleBuildEvent( if testResult.attempt > 1 { outputDir = filepath.Join(outputDir, fmt.Sprintf("attempt_%d", testResult.attempt)) } + if testResult.testResult == nil { + continue + } for _, output := range testResult.testResult.TestActionOutput { if output.Name == "test.log" || output.Name == "test.xml" { src := strings.TrimPrefix(output.GetUri(), "file://") diff --git a/pkg/internal/codeowners/codeowners.go b/pkg/internal/codeowners/codeowners.go index 750a2af9f09a..0a17f19c4af7 100644 --- a/pkg/internal/codeowners/codeowners.go +++ b/pkg/internal/codeowners/codeowners.go @@ -53,7 +53,7 @@ func LoadCodeOwners(r io.Reader, teams map[team.Alias]team.Team) (*CodeOwners, e if s.Err() != nil { return nil, s.Err() } - t := s.Text() + t := strings.Replace(s.Text(), "#!", "", -1) if strings.HasPrefix(t, "#") { continue } diff --git a/pkg/internal/codeowners/codeowners_test.go b/pkg/internal/codeowners/codeowners_test.go index 2094e68c5a92..4d4dc6419887 100644 --- a/pkg/internal/codeowners/codeowners_test.go +++ b/pkg/internal/codeowners/codeowners_test.go @@ -29,11 +29,14 @@ func TestMatch(t *testing.T) { /b/ @cockroachdb/team-b-noreview /a/b* @cockroachdb/team-b @cockroachdb/team-a **/c/ @cockroachdb/team-c +#!/q/ @cockroachdb/team-q +/qq/ @cockroachdb/team-q #! @cockroachdb/team-b-noreview ` teams := map[team.Alias]team.Team{ "cockroachdb/team-a": {}, "cockroachdb/team-b": {}, "cockroachdb/team-c": {}, + "cockroachdb/team-q": {}, } codeOwners, err := LoadCodeOwners(strings.NewReader(owners), teams) @@ -49,6 +52,8 @@ func TestMatch(t *testing.T) { {"a/bob", []team.Team{teams["cockroachdb/team-b"], teams["cockroachdb/team-a"]}}, {"no/owner/", nil}, {"hmm/what/about/c/file", []team.Team{teams["cockroachdb/team-c"]}}, + {"q/foo.txt", []team.Team{teams["cockroachdb/team-q"]}}, + {"qq/foo.txt", []team.Team{teams["cockroachdb/team-q"], teams["cockroachdb/team-b"]}}, } for _, tc := range testCases { diff --git a/pkg/kv/kvserver/BUILD.bazel b/pkg/kv/kvserver/BUILD.bazel index a3de6e70326c..1b7281ef99cb 100644 --- a/pkg/kv/kvserver/BUILD.bazel +++ b/pkg/kv/kvserver/BUILD.bazel @@ -94,7 +94,6 @@ go_library( "stores_base.go", "stores_server.go", "testing_knobs.go", - "track_raft_protos.go", "ts_maintenance_queue.go", ":gen-refreshraftreason-stringer", # keep ], diff --git a/pkg/kv/kvserver/below_raft_protos_test.go b/pkg/kv/kvserver/below_raft_protos_test.go index 88d78e9c91a2..60a8be7cbbf9 100644 --- a/pkg/kv/kvserver/below_raft_protos_test.go +++ b/pkg/kv/kvserver/below_raft_protos_test.go @@ -14,53 +14,32 @@ import ( "fmt" "hash/fnv" "math/rand" - "reflect" + "regexp" "testing" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/storage/enginepb" + "github.com/cockroachdb/cockroach/pkg/testutils" + "github.com/cockroachdb/cockroach/pkg/testutils/echotest" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/protoutil" + "github.com/stretchr/testify/require" "go.etcd.io/etcd/raft/v3/raftpb" ) -func verifyHash(b []byte, expectedSum uint64) error { - hash := fnv.New64a() - if _, err := hash.Write(b); err != nil { - return err - } - if sum := hash.Sum64(); sum != expectedSum { - return fmt.Errorf("expected sum %d; got %d", expectedSum, sum) - } - return nil -} - -// An arbitrary number chosen to seed the PRNGs used to populate the tested -// protos. -const goldenSeed = 1337 - -// The count of randomly populated protos that will be concatenated and hashed -// per proto type. Given that the population functions have a chance of leaving -// some fields zero-valued, this number must be greater than `1` to give this -// test a reasonable chance of encountering a non-zero value of every field. -const itersPerProto = 20 - -type fixture struct { - populatedConstructor func(*rand.Rand) protoutil.Message - emptySum, populatedSum uint64 -} +// TestBelowRaftProtosDontChange is a manually curated list of protos that we +// use below Raft. Care must be taken to change these protos, as replica +// divergence could ensue (if the old code and the new code handle the updated +// or old proto differently). Changes to the encoding of these protos will be +// detected by this test. The expectations should only be updated after a +// reflection on the safety of the proposed change. +func TestBelowRaftProtosDontChange(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) -// belowRaftGoldenProtos are protos that we use below Raft. Care must be -// taken to change these protos, as replica divergence could ensue (if the -// old code and the new code handle the updated or old proto differently). -// To reduce the chances of a bug like this, we track the protos that are -// used below Raft and fail on any changes to their structure. When a -// migration was put into place, the map below can be updated with the new -// emptySum and populatedSum for the proto that was changed. -var belowRaftGoldenProtos = map[reflect.Type]fixture{ - reflect.TypeOf(&enginepb.MVCCMetadata{}): { - populatedConstructor: func(r *rand.Rand) protoutil.Message { + testCases := []func(r *rand.Rand) protoutil.Message{ + func(r *rand.Rand) protoutil.Message { m := enginepb.NewPopulatedMVCCMetadata(r, false) m.Txn = nil // never populated below Raft m.Timestamp.Synthetic = nil // never populated below Raft @@ -70,18 +49,10 @@ var belowRaftGoldenProtos = map[reflect.Type]fixture{ m.TxnDidNotUpdateMeta = nil // never populated below Raft return m }, - emptySum: 7551962144604783939, - populatedSum: 6170112718709472849, - }, - reflect.TypeOf(&enginepb.RangeAppliedState{}): { - populatedConstructor: func(r *rand.Rand) protoutil.Message { + func(r *rand.Rand) protoutil.Message { return enginepb.NewPopulatedRangeAppliedState(r, false) }, - emptySum: 10160370728048384381, - populatedSum: 13858955692092952193, - }, - reflect.TypeOf(&raftpb.HardState{}): { - populatedConstructor: func(r *rand.Rand) protoutil.Message { + func(r *rand.Rand) protoutil.Message { type expectedHardState struct { Term uint64 Vote uint64 @@ -98,81 +69,55 @@ var belowRaftGoldenProtos = map[reflect.Type]fixture{ Commit: n % 11, } }, - emptySum: 13621293256077144893, - populatedSum: 13375098491754757572, - }, - // This is used downstream of Raft only to write it into unreplicated keyspace - // as part of VersionUnreplicatedRaftTruncatedState. - // However, it has been sent through Raft for a long time, as part of - // ReplicatedEvalResult. - reflect.TypeOf(&roachpb.RaftTruncatedState{}): { - populatedConstructor: func(r *rand.Rand) protoutil.Message { + func(r *rand.Rand) protoutil.Message { + // This is used downstream of Raft only to write it into unreplicated keyspace + // as part of VersionUnreplicatedRaftTruncatedState. + // However, it has been sent through Raft for a long time, as part of + // ReplicatedEvalResult. return roachpb.NewPopulatedRaftTruncatedState(r, false) }, - emptySum: 5531676819244041709, - populatedSum: 14781226418259198098, - }, - // These are marshaled below Raft by the Pebble merge operator. The Pebble - // merge operator can be called below Raft whenever a Pebble MVCCIterator is - // used. - reflect.TypeOf(&roachpb.InternalTimeSeriesData{}): { - populatedConstructor: func(r *rand.Rand) protoutil.Message { + func(r *rand.Rand) protoutil.Message { + + // These are marshaled below Raft by the Pebble merge operator. The Pebble + // merge operator can be called below Raft whenever a Pebble MVCCIterator is + // used. return roachpb.NewPopulatedInternalTimeSeriesData(r, false) }, - emptySum: 5531676819244041709, - populatedSum: 17471291891947207032, - }, - reflect.TypeOf(&enginepb.MVCCMetadataSubsetForMergeSerialization{}): { - populatedConstructor: func(r *rand.Rand) protoutil.Message { + func(r *rand.Rand) protoutil.Message { m := enginepb.NewPopulatedMVCCMetadataSubsetForMergeSerialization(r, false) if m.MergeTimestamp != nil { m.MergeTimestamp.Synthetic = nil // never populated below Raft } return m }, - emptySum: 14695981039346656037, - populatedSum: 1187861800212570275, - }, - reflect.TypeOf(&roachpb.RaftReplicaID{}): { - populatedConstructor: func(r *rand.Rand) protoutil.Message { + func(r *rand.Rand) protoutil.Message { return roachpb.NewPopulatedRaftReplicaID(r, false) }, - emptySum: 598336668751268149, - populatedSum: 9313101058286450988, - }, -} - -func TestBelowRaftProtos(t *testing.T) { - defer leaktest.AfterTest(t)() - defer log.Scope(t).Close(t) - - // Enable the additional checks in TestMain. NB: running this test by itself - // will fail those extra checks - such failures are safe to ignore, so long - // as this test passes when run with the entire package's tests. - verifyBelowRaftProtos = true - - slice := make([]byte, 1<<20) - for typ, fix := range belowRaftGoldenProtos { - if b, err := protoutil.Marshal(reflect.New(typ.Elem()).Interface().(protoutil.Message)); err != nil { - t.Fatal(err) - } else if err := verifyHash(b, fix.emptySum); err != nil { - t.Errorf("%s (empty): %s\n", typ, err) - } + } - randGen := rand.New(rand.NewSource(goldenSeed)) + // An arbitrary number chosen to seed the PRNGs used to populate the tested + // protos. + const goldenSeed = 1337 + // We'll randomly populate, marshal, and hash each proto. Doing this more than + // once is necessary to make it very likely that all fields will be nonzero at + // some point. + const itersPerProto = 50 - bytes := slice - numBytes := 0 - for i := 0; i < itersPerProto; i++ { - if n, err := fix.populatedConstructor(randGen).MarshalTo(bytes); err != nil { - t.Fatal(err) - } else { - bytes = bytes[n:] - numBytes += n + w := echotest.NewWalker(t, testutils.TestDataPath(t, t.Name())) + for _, fn := range testCases { + name := fmt.Sprintf("%T", fn(rand.New(rand.NewSource(0)))) + name = regexp.MustCompile(`.*\.`).ReplaceAllString(name, "") + t.Run(name, w.Run(t, name, func(t *testing.T) string { + rng := rand.New(rand.NewSource(goldenSeed)) + hash := fnv.New64a() + for i := 0; i < itersPerProto; i++ { + msg := fn(rng) + dst := make([]byte, msg.Size()) + _, err := msg.MarshalTo(dst) + require.NoError(t, err) + hash.Write(dst) } - } - if err := verifyHash(slice[:numBytes], fix.populatedSum); err != nil { - t.Errorf("%s (populated): %s\n", typ, err) - } + return fmt.Sprint(hash.Sum64()) + })) } } diff --git a/pkg/kv/kvserver/main_test.go b/pkg/kv/kvserver/main_test.go index ef31b304a8f1..a529c77a6e2b 100644 --- a/pkg/kv/kvserver/main_test.go +++ b/pkg/kv/kvserver/main_test.go @@ -11,20 +11,14 @@ package kvserver_test import ( - "fmt" "os" - "reflect" "testing" - "github.com/cockroachdb/cockroach/pkg/kv/kvserver" - "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security/securityassets" "github.com/cockroachdb/cockroach/pkg/security/securitytest" "github.com/cockroachdb/cockroach/pkg/server" - "github.com/cockroachdb/cockroach/pkg/storage/enginepb" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" - "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/randutil" ) @@ -34,69 +28,9 @@ func init() { securityassets.SetLoader(securitytest.EmbeddedAssets) } -var verifyBelowRaftProtos bool - func TestMain(m *testing.M) { randutil.SeedForTests() serverutils.InitTestServerFactory(server.TestServerFactory) serverutils.InitTestClusterFactory(testcluster.TestClusterFactory) - - // The below-Raft proto tracking is fairly expensive in terms of allocations - // which significantly impacts the tests under -race. We're already doing the - // below-Raft proto tracking in non-race builds, so there is little benefit - // to also doing it in race builds. - if util.RaceEnabled { - os.Exit(m.Run()) - } - - // Create a set of all protos we believe to be marshaled downstream of raft. - // After the tests are run, we'll subtract the encountered protos from this - // set. - notBelowRaftProtos := make(map[reflect.Type]struct{}, len(belowRaftGoldenProtos)) - for typ := range belowRaftGoldenProtos { - notBelowRaftProtos[typ] = struct{}{} - } - - // Before running the tests, enable instrumentation that tracks protos which - // are marshaled downstream of raft. - stopTrackingAndGetTypes := kvserver.TrackRaftProtos() - - code := m.Run() - - // Only do this verification if the associated test was run. Without this - // condition, the verification here would spuriously fail when running a - // small subset of tests e.g. as we often do with `stress`. - if verifyBelowRaftProtos { - failed := false - // Retrieve all the observed downstream-of-raft protos and confirm that they - // are all present in our expected set. - for _, typ := range stopTrackingAndGetTypes() { - if _, ok := belowRaftGoldenProtos[typ]; ok { - delete(notBelowRaftProtos, typ) - } else { - failed = true - fmt.Printf("%s: missing fixture! Please adjust belowRaftGoldenProtos if necessary\n", typ) - } - } - - // Confirm that our expected set is now empty; we don't want to cement any - // protos needlessly. - // Two exceptions: these are sometimes observed below raft, sometimes not. - // It supposedly depends on whether any test server runs for long enough to - // write internal time series. - delete(notBelowRaftProtos, reflect.TypeOf(&roachpb.InternalTimeSeriesData{})) - delete(notBelowRaftProtos, reflect.TypeOf(&enginepb.MVCCMetadataSubsetForMergeSerialization{})) - for typ := range notBelowRaftProtos { - // NB: don't set failed=true. In a bazel world, we may just end up sharding in a way that - // doesn't observe some of the protos below raft. - fmt.Printf("%s: not observed below raft!\n", typ) - } - - // Make sure our error messages make it out. - if failed && code == 0 { - code = 1 - } - } - - os.Exit(code) + os.Exit(m.Run()) } diff --git a/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/HardState b/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/HardState new file mode 100644 index 000000000000..a1ea2ff7ced1 --- /dev/null +++ b/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/HardState @@ -0,0 +1,3 @@ +echo +---- +5484201021249543052 diff --git a/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/InternalTimeSeriesData b/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/InternalTimeSeriesData new file mode 100644 index 000000000000..c3211d1221de --- /dev/null +++ b/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/InternalTimeSeriesData @@ -0,0 +1,3 @@ +echo +---- +17722245750004537461 diff --git a/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/MVCCMetadata b/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/MVCCMetadata new file mode 100644 index 000000000000..beddd2fd2502 --- /dev/null +++ b/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/MVCCMetadata @@ -0,0 +1,3 @@ +echo +---- +15541461448355390613 diff --git a/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/MVCCMetadataSubsetForMergeSerialization b/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/MVCCMetadataSubsetForMergeSerialization new file mode 100644 index 000000000000..cd042b3d7e72 --- /dev/null +++ b/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/MVCCMetadataSubsetForMergeSerialization @@ -0,0 +1,3 @@ +echo +---- +12542053708208219209 diff --git a/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/RaftReplicaID b/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/RaftReplicaID new file mode 100644 index 000000000000..ed169c41c817 --- /dev/null +++ b/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/RaftReplicaID @@ -0,0 +1,3 @@ +echo +---- +9376155980184170514 diff --git a/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/RaftTruncatedState b/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/RaftTruncatedState new file mode 100644 index 000000000000..216c365a7e28 --- /dev/null +++ b/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/RaftTruncatedState @@ -0,0 +1,3 @@ +echo +---- +17799557460885784691 diff --git a/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/RangeAppliedState b/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/RangeAppliedState new file mode 100644 index 000000000000..8d3e44f5616a --- /dev/null +++ b/pkg/kv/kvserver/testdata/TestBelowRaftProtosDontChange/RangeAppliedState @@ -0,0 +1,3 @@ +echo +---- +14062697193383087404 diff --git a/pkg/kv/kvserver/track_raft_protos.go b/pkg/kv/kvserver/track_raft_protos.go deleted file mode 100644 index fbd58a287346..000000000000 --- a/pkg/kv/kvserver/track_raft_protos.go +++ /dev/null @@ -1,118 +0,0 @@ -// Copyright 2016 The Cockroach Authors. -// -// Use of this software is governed by the Business Source License -// included in the file licenses/BSL.txt. -// -// As of the Change Date specified in that file, in accordance with -// the Business Source License, use of this software will be governed -// by the Apache License, Version 2.0, included in the file -// licenses/APL.txt. - -package kvserver - -import ( - "fmt" - "reflect" - "runtime" - "strings" - - "github.com/cockroachdb/cockroach/pkg/gossip" - "github.com/cockroachdb/cockroach/pkg/kv/kvserver/apply" - "github.com/cockroachdb/cockroach/pkg/storage/enginepb" - "github.com/cockroachdb/cockroach/pkg/util/protoutil" - "github.com/cockroachdb/cockroach/pkg/util/syncutil" -) - -func funcName(f interface{}) string { - return runtime.FuncForPC(reflect.ValueOf(f).Pointer()).Name() -} - -// TrackRaftProtos instruments proto marshaling to track protos which are -// marshaled downstream of raft. It returns a function that removes the -// instrumentation and returns the list of downstream-of-raft protos. -func TrackRaftProtos() func() []reflect.Type { - // Grab the name of the function that roots all raft application. - applyRaftEntryFunc := funcName((*apply.Task).ApplyCommittedEntries) - // We only need to track protos that could cause replica divergence - // by being written to disk downstream of raft. - allowlist := []string{ - // Some raft operations trigger gossip, but we don't require - // strict consistency there. - funcName((*gossip.Gossip).AddInfoProto), - // Range merges destroy replicas beneath Raft and write replica tombstones, - // but tombstones are unreplicated and thus not subject to the strict - // consistency requirements. - funcName((*Replica).setTombstoneKey), - // tryReproposeWithNewLeaseIndex is only run on the replica that - // proposed the command. - funcName((*Replica).tryReproposeWithNewLeaseIndex), - } - - belowRaftProtos := struct { - syncutil.Mutex - inner map[reflect.Type]struct{} - }{ - inner: make(map[reflect.Type]struct{}), - } - - protoutil.Interceptor = func(pb protoutil.Message) { - t := reflect.TypeOf(pb) - - // Special handling for MVCCMetadata: we expect MVCCMetadata to be - // marshaled below raft, but MVCCMetadata.Txn should always be nil in such - // cases. - if meta, ok := pb.(*enginepb.MVCCMetadata); ok && meta.Txn != nil { - protoutil.Interceptor(meta.Txn) - } - - belowRaftProtos.Lock() - _, ok := belowRaftProtos.inner[t] - belowRaftProtos.Unlock() - if ok { - return - } - - var pcs [256]uintptr - if numCallers := runtime.Callers(0, pcs[:]); numCallers == len(pcs) { - panic(fmt.Sprintf("number of callers %d might have exceeded slice size %d", numCallers, len(pcs))) - } - frames := runtime.CallersFrames(pcs[:]) - for { - f, more := frames.Next() - - allowlisted := false - for _, s := range allowlist { - if strings.Contains(f.Function, s) { - allowlisted = true - break - } - } - if allowlisted { - break - } - - if strings.Contains(f.Function, applyRaftEntryFunc) { - belowRaftProtos.Lock() - belowRaftProtos.inner[t] = struct{}{} - belowRaftProtos.Unlock() - break - } - if !more { - break - } - } - } - - return func() []reflect.Type { - protoutil.Interceptor = func(_ protoutil.Message) {} - - belowRaftProtos.Lock() - types := make([]reflect.Type, 0, len(belowRaftProtos.inner)) - for t := range belowRaftProtos.inner { - types = append(types, t) - } - belowRaftProtos.Unlock() - - return types - } -} diff --git a/pkg/sql/opt/memo/testdata/stats/scan b/pkg/sql/opt/memo/testdata/stats/scan index 14c153131f93..0da58c2130e2 100644 --- a/pkg/sql/opt/memo/testdata/stats/scan +++ b/pkg/sql/opt/memo/testdata/stats/scan @@ -1091,16 +1091,16 @@ project ├── fd: ()-->(6) ├── index-join t47742 │ ├── columns: a:1(int) t47742.b:2(bool!null) - │ ├── stats: [rows=3063.5, distinct(2)=1.0025, null(2)=0] - │ │ histogram(2)= 0 0 0.0024693 3063.5 - │ │ <--- false ----------- true + │ ├── stats: [rows=3063.5, distinct(2)=1, null(2)=0] + │ │ histogram(2)= 0 3063.5 + │ │ <--- true │ ├── fd: ()-->(2) │ └── scan t47742@b_idx │ ├── columns: t47742.b:2(bool!null) rowid:3(int!null) │ ├── constraint: /-2/3: [/true - /true] - │ ├── stats: [rows=3063.5, distinct(2)=11, null(2)=0] - │ │ histogram(2)= 0 0 0.0024693 3063.5 - │ │ <--- NULL ----------- true + │ ├── stats: [rows=3063.5, distinct(2)=1, null(2)=0] + │ │ histogram(2)= 0 3063.5 + │ │ <--- true │ ├── key: (3) │ └── fd: ()-->(2) └── projections diff --git a/pkg/sql/opt/props/histogram.go b/pkg/sql/opt/props/histogram.go index 3fb3c3d4dcba..24c424cf3e32 100644 --- a/pkg/sql/opt/props/histogram.go +++ b/pkg/sql/opt/props/histogram.go @@ -328,8 +328,18 @@ func (h *Histogram) filter( if filteredSpan.Compare(&keyCtx, &left) != 0 { // The bucket was cut off in the middle. Get the resulting filtered // bucket. + // + // The span generated from the bucket may have an exclusive bound in + // order to avoid a datum allocation for the majority of cases where + // the span does not intersect the bucket. If the span does + // intersect with the bucket, we transform the bucket span into an + // inclusive one to make it easier to work with. Note that this + // conversion is best-effort; not all datums support Next or Prev + // which allow exclusive ranges to be converted to inclusive ones. + cmpStarts := filteredSpan.CompareStarts(&keyCtx, &left) + filteredSpan.PreferInclusive(&keyCtx) filteredBucket = getFilteredBucket(&iter, &keyCtx, &filteredSpan, colOffset) - if !desc && filteredSpan.CompareStarts(&keyCtx, &left) != 0 { + if !desc && cmpStarts != 0 { // We need to add an empty bucket before the new bucket. ub := h.getPrevUpperBound(filteredSpan.StartKey(), filteredSpan.StartBoundary(), colOffset) filtered.addEmptyBucket(ub, desc) @@ -362,7 +372,7 @@ func (h *Histogram) filter( // bucket if needed. if iter.next() { // The remaining buckets from the original histogram have been removed. - filtered.addEmptyBucket(iter.lb, desc) + filtered.addEmptyBucket(iter.inclusiveLowerBound(), desc) } else if lastBucket := filtered.buckets[len(filtered.buckets)-1]; lastBucket.NumRange != 0 { iter.setIdx(0) span := makeSpanFromBucket(&iter, prefix) @@ -496,8 +506,10 @@ type histogramIter struct { desc bool idx int b *cat.HistogramBucket - lb tree.Datum - ub tree.Datum + elb tree.Datum // exclusive lower bound + lb tree.Datum // inclusive lower bound + eub tree.Datum // exclusive upper bound + ub tree.Datum // inclusive upper bound } // init initializes a histogramIter to point to the first bucket of the given @@ -531,15 +543,18 @@ func (hi *histogramIter) setIdx(i int) { // the "next" bucket is actually the previous bucket in the histogram. Returns // false if there are no more buckets. func (hi *histogramIter) next() (ok bool) { - getBounds := func() (lb, ub tree.Datum) { + getBounds := func() (elb, lb, eub, ub tree.Datum) { hi.b = &hi.h.buckets[hi.idx] ub = hi.b.UpperBound if hi.idx == 0 { lb = ub } else { - lb = hi.h.getNextLowerBound(hi.h.upperBound(hi.idx - 1)) + elb = hi.h.upperBound(hi.idx - 1) } - return lb, ub + // We return an exclusive upper bound, eub, even though it is always nil + // because it makes it easier to handle both the iter.desc=true and + // iter.desc=false cases below. + return elb, lb, nil /* eub */, ub } if hi.desc { @@ -547,24 +562,83 @@ func (hi *histogramIter) next() (ok bool) { if hi.idx < 0 { return false } - hi.ub, hi.lb = getBounds() + // If iter.desc=true, the lower bounds are greater than the upper + // bounds. Either hi.eub or hi.ub will be set, hi.elb will always be + // nil, and hi.lb will always be non-nil. + hi.eub, hi.ub, hi.elb, hi.lb = getBounds() } else { hi.idx++ if hi.idx >= hi.h.bucketCount() { return false } - hi.lb, hi.ub = getBounds() + // If iter.desc=false, the lower bounds are less than the upper bounds. + // Either hi.elb or hi.lb will be set, hi.eub will always be nil, and + // hi.ub will always be non-nil. + hi.elb, hi.lb, hi.eub, hi.ub = getBounds() } return true } +// lowerBound returns the lower bound of the current bucket, and whether the +// bound is inclusive or exclusive. It will never allocate a new datum. +func (hi *histogramIter) lowerBound() (tree.Datum, constraint.SpanBoundary) { + if hi.lb != nil { + return hi.lb, constraint.IncludeBoundary + } + return hi.elb, constraint.ExcludeBoundary +} + +// inclusiveLowerBound returns the inclusive lower bound of the current bucket. +// It may allocate a new datum. +func (hi *histogramIter) inclusiveLowerBound() tree.Datum { + if hi.lb == nil { + // hi.lb is only nil if iter.desc=false (see histogramIter.next), which + // means that the lower bounds are less than the upper bounds. So the + // inclusive lower bound is greater than the exclusive lower bound. For + // example, the range (/10 - /20] is equivalent to [/11 - /20]. + hi.lb = hi.h.getNextLowerBound(hi.elb) + } + return hi.lb +} + +// upperBound returns the upper bound of the current bucket, and whether the +// bound is inclusive or exclusive. It will never allocate a new datum. +func (hi *histogramIter) upperBound() (tree.Datum, constraint.SpanBoundary) { + if hi.ub != nil { + return hi.ub, constraint.IncludeBoundary + } + return hi.eub, constraint.ExcludeBoundary +} + +// inclusiveUpperBound returns the inclusive upper bound of the current bucket. +// It may allocate a new datum. +func (hi *histogramIter) inclusiveUpperBound() tree.Datum { + if hi.ub == nil { + // hi.ub is only nil if iter.desc=true (see histogramIter.next), which + // means that the lower bounds are greater than the upper bounds. So the + // inclusive upper bound is greater than the exclusive upper bound. For + // example, the range [/20 - /10) is equivalent to [/20 - /11]. + hi.ub = hi.h.getNextLowerBound(hi.eub) + } + return hi.ub +} + func makeSpanFromBucket(iter *histogramIter, prefix []tree.Datum) (span constraint.Span) { + start, startBoundary := iter.lowerBound() + end, endBoundary := iter.upperBound() + if start.Compare(iter.h.evalCtx, end) == 0 && + (startBoundary == constraint.IncludeBoundary || endBoundary == constraint.IncludeBoundary) { + // If the start and ends are equal and one of the boundaries is + // inclusive, the other boundary should be inclusive. + startBoundary = constraint.IncludeBoundary + endBoundary = constraint.IncludeBoundary + } span.Init( - constraint.MakeCompositeKey(append(prefix[:len(prefix):len(prefix)], iter.lb)...), - constraint.IncludeBoundary, - constraint.MakeCompositeKey(append(prefix[:len(prefix):len(prefix)], iter.ub)...), - constraint.IncludeBoundary, + constraint.MakeCompositeKey(append(prefix[:len(prefix):len(prefix)], start)...), + startBoundary, + constraint.MakeCompositeKey(append(prefix[:len(prefix):len(prefix)], end)...), + endBoundary, ) return span } @@ -605,8 +679,8 @@ func getFilteredBucket( ) *cat.HistogramBucket { spanLowerBound := filteredSpan.StartKey().Value(colOffset) spanUpperBound := filteredSpan.EndKey().Value(colOffset) - bucketLowerBound := iter.lb - bucketUpperBound := iter.ub + bucketLowerBound := iter.inclusiveLowerBound() + bucketUpperBound := iter.inclusiveUpperBound() b := iter.b // Check that the given span is contained in the bucket. diff --git a/pkg/util/protoutil/marshal.go b/pkg/util/protoutil/marshal.go index 0876a5c38865..d551354685b8 100644 --- a/pkg/util/protoutil/marshal.go +++ b/pkg/util/protoutil/marshal.go @@ -22,12 +22,7 @@ type Message interface { Size() int } -// Interceptor will be called with every proto before it is marshaled. -// Interceptor is not safe to modify concurrently with calls to Marshal. -var Interceptor = func(_ Message) {} - -// Marshal encodes pb into the wire format. It is used throughout the code base -// to intercept calls to proto.Marshal. +// Marshal encodes pb into the wire format. func Marshal(pb Message) ([]byte, error) { dest := make([]byte, pb.Size()) if _, err := MarshalToSizedBuffer(pb, dest); err != nil { @@ -36,17 +31,13 @@ func Marshal(pb Message) ([]byte, error) { return dest, nil } -// MarshalTo encodes pb into the wire format. It is used throughout the code -// base to intercept calls to pb.MarshalTo. +// MarshalTo encodes pb into the wire format. func MarshalTo(pb Message, dest []byte) (int, error) { - Interceptor(pb) return pb.MarshalTo(dest) } -// MarshalToSizedBuffer encodes pb into the wire format. It is used -// throughout the code base to intercept calls to pb.MarshalToSizedBuffer. +// MarshalToSizedBuffer encodes pb into the wire format. func MarshalToSizedBuffer(pb Message, dest []byte) (int, error) { - Interceptor(pb) return pb.MarshalToSizedBuffer(dest) }