Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clusterversion: introduce 22.1 development versions #69828

Merged
merged 1 commit into from
Oct 19, 2021

Conversation

celiala
Copy link
Collaborator

@celiala celiala commented Sep 3, 2021

Shortly after Creating a release branch for release-21.2, we'll want to merge PRs as instructed by the New major version checklist.

This PR is for Step 1b of the New major version checklist, where we add a cluster version VersionStart${vNEXT}.

This is part of #70751, which tracks all the steps relevant to creating a release branch for ${vBRANCH_CUT} and preparing master for the ${vNEXT} major version.

Release justification: Non-production code change.
Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@celiala celiala added the do-not-merge bors won't merge a PR with this label. label Sep 3, 2021
@celiala
Copy link
Collaborator Author

celiala commented Sep 3, 2021

This should not be merged until after Tuesday's branch cut.

Update:
This should NOT be merged until we've released a beta (adding do-not-merge until this happens)

@celiala celiala changed the title clusterversion: mint 21.2 cluster version clusterversion: introduce 22.1 development versions Sep 3, 2021
@@ -1,4 +1,5 @@
// Code generated by "stringer -type=Key"; DO NOT EDIT.
// TODO(celia) -- how do I re-autogenerate this file?
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO(celia) - who might know the answer to this?

{
Key: Start21_2,

// TODO(celia) - what should `Internal` be?
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe @RaduBerinde ?

Version: roachpb.Version{Major: 21, Minor: 1, Internal: 200},
},

// TODO(celia) - should I be making a Start21_2PLUS, similar to
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe @RaduBerinde here as well?

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @celiala, and @irfansharif)


pkg/clusterversion/cockroach_versions.go, line 477 at r4 (raw file):

	// TODO(celia) - should `Internal` be 1160+2? Or should a use something bigger like 2000?
	{
		Key:     Start21_2,

Start22_1


pkg/clusterversion/cockroach_versions.go, line 478 at r4 (raw file):

	{
		Key:     Start21_2,
		Version: roachpb.Version{Major: 21, Minor: 1, Internal: 2000},

Major 22 Minor 1. Internal can be (say) 100, we just want to leave a gap in case we need it later. These values are never compared against internal values from a different Major/Minor.


pkg/clusterversion/cockroach_versions.go, line 481 at r4 (raw file):

	},

	// TODO(celia) - should I be making a Start21_2PLUS, similar to Start21_1PLUS for serverless?

No PLUS, ignore that. We didn't end up doing that.


pkg/clusterversion/key_string.go, line 2 at r2 (raw file):

Previously, celiala wrote…

TODO(celia) - who might know the answer to this?

go generate in that dir should do it I think. Or make generate

// v21.2 versions.
// TODO(celia) - should `Internal` be 1160+2? Or should a use something bigger like 2000?
{
Key: Start21_2,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to use Start22_1 here; Start21_2 is already defined above. Also, the version should be roachpb.Version{Major: 21, Minor: 2, Internal: 2}. The +14, +100, and +1000 were due to historical artifacts. Pretty much any number >= 2 would work here, but I think we should start at 2.

Version: roachpb.Version{Major: 21, Minor: 1, Internal: 2000},
},

// TODO(celia) - should I be making a Start21_2PLUS, similar to Start21_1PLUS for serverless?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think so.

@irfansharif
Copy link
Contributor

(The LGTM was for code changes -- not sure when this PR is supposed to get merged. Looking at the discussion over at #69826, doesn't seem like it would be now)

@celiala
Copy link
Collaborator Author

celiala commented Sep 9, 2021

TFTR!

(The LGTM was for code changes -- not sure when this PR is supposed to get merged. Looking at the discussion over at #69826, doesn't seem like it would be now)

Yes, I'll hold off merging this until we've released a beta. I updated the PR description to clarify when I intend to merge - thanks!

@irfansharif
Copy link
Contributor

When we merge this, we also want to bump the binaryMinSupportedVersion to point to V21_2 instead.

@irfansharif
Copy link
Contributor

// (b) When cutting a major release branch. When cutting release-20.2 for
// example, you'll want to introduce the following to `master`.
//
// (i) V20_2 (keyed to v20.2.0-0})
// (ii) Start21_1 (keyed to v20.2.0-1})
//
// You'll then want to backport (i) to the release branch itself (i.e.
// release-20.2). You'll also want to bump binaryMinSupportedVersion. In the
it's kind of tucked away here, is there a TODO list you're maintaining elsewhere Celia?

irfansharif added a commit to irfansharif/cockroach that referenced this pull request Sep 15, 2021
Fixes cockroachdb#65200. The last remaining 21.1 version (V21_1) can be removed as
part of cockroachdb#69828.

Release note: None
craig bot pushed a commit that referenced this pull request Sep 15, 2021
68282: cli,log: allow use of `debug merge-logs` on older logs r=knz a=ajwerner

Fixes [#68278](#68278).

Log parsers require the flag `--format` when parsing older logs (because 
they do not contain format specification). With this patch, this is no longer 
a requirement as the log format is now inferred based on the structure of 
the log if no log format specification exists.

Release justification: bug fix

Release note (bug fix): The debug merge-logs command no longer returns an error 
when the log decoder attempts to parse older logs.

69903: importccl: add support for IMPORT INTO RBR table r=arulajmani,ajstorm,dt a=adityamaru

This change overrides the `default_to_database_primary_region`
and `gateway_region` to always return the primary region of the
database of the table being imported into. This allows for
IMPORT INTO an RBR table.

To ensure that the import is idempotent across resumptions, we cache
the primary region of the database being imported into, during planning.
This information is store in the job details and flow spec to be used
when evaluating the relevant default expr/computed column.

Since IMPORT is a job, it does not have an associated session data
and so it cannot rely on the planners' implementation of the regional
operator. This change also implements the relevant methods in the
`importRegionOperator` to allow resolution of the primary region
of the database being imported into.

Fixes: #69616

Release note (sql change): IMPORT INTO regional by row tables
is supported.

Release justification: fixes for high-priority or high-severity bugs in existing functionality

70150: server: fix TestAdminAPIJobs failure r=knz a=adityamaru

This change sorts the expected job IDs before ensuring
that they are equal.

Fixes: #69401

Release note: None

70226: changefeedccl: updated retryable error warning message r=wongio123 a=wongio123

Retryable error warning message contained the word "error"
Confusing to users because warning message had the word "error" in it
Prefaced warning message with "WARNING"

Release note (enterprise change): updated retyable error warning message to begin with "WARNING"

Closes #69677 

70229: [CRDB-9016] ui: fix drag to zoom on custom charts r=Santamaura a=Santamaura

This PR addresses the issue where a user creates a custom chart and selects an area to zoom into which leaves the grey highlight after the graph zooms in. This was due to the history prop not being passed into the linegraph component and caused an error to throw when updating the url params. This was resolved by passing in the history to propagate to the
linegraph component.

Release note (ui change): fix drag to zoom on custom charts

https://user-images.githubusercontent.com/17861665/133342585-d7b37e9b-7eb8-4a48-b2c5-814fed62556a.mp4



70262: ui: add column selector to transation page r=maryliag a=maryliag

Add column selector to Transaction Page

Fixes #70148

<img width="414" alt="Screen Shot 2021-09-15 at 11 28 56 AM" src="https://user-images.githubusercontent.com/1017486/133463202-7ed7ac3a-9614-4101-ad76-8f431defe688.png">


Release justification: Category 4
Release note (ui change): Add column selector to transaction page

70268: clusterversion: remove Start21_1 (no longer applicable) r=irfansharif a=irfansharif

Fixes #65200. The last remaining 21.1 version (V21_1) can be removed as
part of #69828.

Release note: None

Co-authored-by: Andrew Werner <awerner32@gmail.com>
Co-authored-by: Aditya Maru <adityamaru@gmail.com>
Co-authored-by: Alex Wong <alex.wong@cockroachlabs.com>
Co-authored-by: Santamaura <santamaura@cockroachlabs.com>
Co-authored-by: Marylia Gutierrez <marylia@cockroachlabs.com>
Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
craig bot pushed a commit that referenced this pull request Oct 1, 2021
69826: clusterversion: mint 21.2 cluster version r=celiala a=celiala

Shortly after [Creating a release branch](https://cockroachlabs.atlassian.net/wiki/spaces/ENG/pages/187859111/Creating+a+release+branch) for release-21.2, we'll want to merge PRs as instructed by the [New major version checklist](https://cockroachlabs.atlassian.net/wiki/spaces/ENG/pages/1522270228/New+major+version+checklist).

This PR is for Step 1a of the [New major version checklist](https://cockroachlabs.atlassian.net/wiki/spaces/ENG/pages/1522270228/New+major+version+checklist), where we add a cluster version StartX for the corresponding .0 release.

Notes: 
- **This should NOT be merged until we've released a beta** (adding `do-not-merge` until this happens)
- These are all PRs created as part of the steps of the [New major version checklist](https://cockroachlabs.atlassian.net/wiki/spaces/ENG/pages/1522270228/New+major+version+checklist):
	- Step 1a (you are here): #69826
	- Step 1b: #69828
	- Step 2a + Step 2b: #69827
	- Step 2c: #69829
	- Step 3: TODO

Release justification: Non-production code change.
Release note: None

70750: tenant: add endpoint with instant metrics r=darinpp a=darinpp

Previously the tenant process was serving various metrics on
`/_status/vars`. This endpoint has all the available metrics and these are
updated every 10 sec. Many of the metrics show a rate that is calculated
over the 10 sec interval. Some of the metrics are used by the cockroach
operator to monitor the CPU workload of the tenant process and use that
workload for automatic scaling. The 10 sec interval however is too long
and causes a slow scaling up. The reporting of high CPU utilization can
take up to 20 sec (to compute a delta). To resolve this, the PR adds a
new endpoint `/_status/load` that provides an instant reading of a
very small subset of the normal metrics - user and system CPU time for
now. By having these be instant, the client can retrieve in quick
succession, consecutive snapshots and compute a precise CPU utulization.
It also allows the client to control the interval between the two pulls
(as opposed to having it hard coded to 10 sec).

Release note: None

Co-authored-by: Celia La <celiala456@gmail.com>
Co-authored-by: Darin Peshev <darinp@gmail.com>
@celiala celiala force-pushed the introduce-21.2-dev-versions branch from 63b7dd4 to 6ee0d29 Compare October 1, 2021 16:19
Copy link
Collaborator Author

@celiala celiala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also want to bump the binaryMinSupportedVersion to point to V21_2 instead.

Ack. Done.

[the steps in the comments are] kind of tucked away here, is there a TODO list you're maintaining elsewhere Celia?

Ack, good callout!

Yes. We currently have these wiki pages that I've been running through:

But with "multiple sources of truth" there's a chance that variations will happen. I have a running list of things to improve[]1 for the next branch cut -- I'll add this "multiple sources of truth" as something to consolidate. Thanks!

[1] https://cockroachlabs.atlassian.net/wiki/spaces/devinf/pages/2218197098/21.2+release+process+notes+a+running+list+retro

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @irfansharif)


pkg/clusterversion/cockroach_versions.go, line 477 at r4 (raw file):

Previously, irfansharif (irfan sharif) wrote…

I think we want to use Start22_1 here; Start21_2 is already defined above. Also, the version should be roachpb.Version{Major: 21, Minor: 2, Internal: 2}. The +14, +100, and +1000 were due to historical artifacts. Pretty much any number >= 2 would work here, but I think we should start at 2.

Done.


pkg/clusterversion/cockroach_versions.go, line 481 at r4 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Don't think so.

Done.

@celiala
Copy link
Collaborator Author

celiala commented Oct 1, 2021

FYI - I plan to hold off on merging this until we have a solid/stable RC, as per conversation from

#69826 (comment)

@celiala
Copy link
Collaborator Author

celiala commented Oct 18, 2021

🤔 I'm seeing this pattern of error:

E211001 16:44:27.695542 51 server/init.go:326  [n?] 18  bootstrap: store &{engine1 false <no-attributes>=<in-mem> 0xc0004a5260}, last used with cockroach version v21.1-1132, is too old for running version v21.1-1132 (which requires data from v21.2 or later)
    testcluster.go:307: unable to bootstrap due to internal error

On a handful of tests:

  • pkg/ccl/migrationccl/migrationsccl/
    • TestRecordsBasedRegistryMigration
  • pkg/migration/migrations/
    • TestDeleteDeprecatedNamespaceDescriptorMigration
    • TestDeleteDeprecatedNamespaceDescriptorMigrationOnlyNamespace2
    • TestDeleteDeprecatedNamespaceDescriptorMigrationNoOp
      ...

It sounds like this needs to pull a v21.2-x build, but I'm not sure how these tests get setup, or how to update that..

Ideas?

@celiala celiala removed the do-not-merge bors won't merge a PR with this label. label Oct 18, 2021
irfansharif added a commit to irfansharif/cockroach that referenced this pull request Oct 19, 2021
Makes for a more helpful error message in cockroachdb#69828; there we're dealing
with tests that override the binary version that now fail seeing as how
the min supported version is advanced.

Release note: None
@irfansharif
Copy link
Contributor

irfansharif commented Oct 19, 2021

So these tests in the migration{,ccl} packages override the binary version in order to test the execution of a specific migration association with a specific cluster version.

BinaryVersionOverride: clusterversion.ByKey(clusterversion.DeleteDeprecatedNamespaceTableDescriptorMigration - 1),

Given that this PR bumps the minimum supported binary version, for these tests we're in the incorrect state where the binary version is actually less than the minimum supported version (#71684 would've made this clearer). One thing we could do is also these tests override the minimum supported version. I've paged most of this out, but I think the way we'd do it by upgrading all tests that directly touch BinaryVersionOverride to instead use a mocked out clusterversion.Handle instead, threading that in appropriately:

// TODO(irfansharif): Update users of this testing knob to use the
// appropriate clusterversion.Handle instead.
BinaryVersionOverride roachpb.Version

Failed attempt (#71686 + this PR rebased on top of it): I tried a quick revision adding a BinaryMinimumVersionOverride, a sister testing knob to BinaryVersionOverride, but it doesn't play super well with the fact that the cluster version setting is initialized globally) using the real min-supported version, which prevents tests from easily overriding the server's view of min-supported versions because it's checked against the cluster setting. At that point we might as well plumb in a testing knob for a clusterversion.Handle instead. If this paragraph is unreadable, I don't blame you.


@ajwerner, is there another workaround we're missing? @celiala, I'd be ok with skipping the bump of the min supported version but adding a TODO+filing an issue to come back around to it after addressing the above. We'd need to get to it before any 22.1 release.

irfansharif added a commit to irfansharif/cockroach that referenced this pull request Oct 19, 2021
Makes for a more helpful error message in cockroachdb#69828; there we're dealing
with tests that override the binary version that now fail seeing as how
the min supported version is advanced.

Release note: None
irfansharif added a commit to irfansharif/cockroach that referenced this pull request Oct 19, 2021
…rsion

Certain tests override the binary version (for e.g. in order to test the
execution of a specific migration association with a specific cluster
version). For these tests we also want to bypass the bootstrap ("is too
old for running version") version check, especially as we bump the
minimum supported version past a bunch of cluster versions that were
introduced in the last release cycle (cockroachdb#69828).

We could technically delete all the past-release cluster version
handling code before introducing next release development versions,
but it's easy to keep it around to simplify backports (and also it's a
lot of code to get rid of, only to allow next-release versions to be
added). Allowing these tests to override the minimum supported
version is a convenient workaround.

---

This commit updates one of the many failing tests blocking cockroachdb#69828, but
not all of them. The approach in this commit of providing a sister testing
knob `BinaryMinSupportedVersion` might not be sufficient due to
interactions with the cluster version setting (see comments on cockroachdb#69828).
It's possible we want to go all the way and all tests to install a
clusterversion.Handle instead. Leaving this PR up for posterity.

Release note: None
@ajwerner
Copy link
Contributor

@ajwerner, is there another workaround we're missing? @celiala, I'd be ok with skipping the bump of the min supported version but adding a TODO+filing an issue to come back around to it after addressing the above. We'd need to get to it before any 22.1 release.

Yeah, but let's definitely file the issue. Not bumping the min supported version is what we did last time but it took us until very late in the cycle to remember to do it.

@irfansharif
Copy link
Contributor

Yeah, but let's definitely file the issue.

#71708

@irfansharif
Copy link
Contributor

(Peeked at the CI failure -- I think we're missing a make generate which updates some of these autogenerated files the CI is checking for.)

craig bot pushed a commit that referenced this pull request Oct 19, 2021
71670: tree: make "rename" stmt tags match postgres r=otan a=rafiss

This makes ALTER statements that rename objects use the correct
statement tag to match Postgres.

Release note: None

71684: server: add assertion around binary versions r=irfansharif a=irfansharif

Makes for a more helpful error message in #69828; there we're dealing
with tests that override the binary version that now fail seeing as how
the min supported version is advanced.

Release note: None

Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
Release justification: Non-production code change.
Release note: None
@celiala
Copy link
Collaborator Author

celiala commented Oct 19, 2021

All tests pass 🎉 -- TFTRs and debugging help!

:shipit:

bors r+

@craig
Copy link
Contributor

craig bot commented Oct 19, 2021

Build succeeded:

@craig craig bot merged commit c8c186f into cockroachdb:master Oct 19, 2021
@celiala
Copy link
Collaborator Author

celiala commented Oct 21, 2021


pkg/clusterversion/cockroach_versions.go, line 477 at r4 (raw file):

I think we want to use Start22_1 here; Start21_2 is already defined above. Also, the version should be roachpb.Version{Major: 21, Minor: 2, Internal: 2}.

Doh 🤦

I totally missed that Major: 21, Minor: 2 should be the previous version to Start22_1.

Fixed in #71836

craig bot pushed a commit that referenced this pull request Oct 22, 2021
71632: sql: make sure pgwire bind always happens in a transaction r=otan a=rafiss

fixes #70378
and maybe #64140

The approach of using a transaction matches what we do for pgwire Parse
messages already. This is important to make sure that user-defined types
are leased correctly.

This also updated the SQL EXECUTE command to resolve user-defined types
so that it gets the latest changes.

Release note (bug fix): Adding new values to a user-defined enum type
will previously would cause a prepared statement using that type to not
work. This now works as expected.

71836: clusterversion: introduce 22.1 development versions r=celiala a=celiala

Fix: #69828

Looking at past Version values for `Start{XX_X}`, I think the `Version` value for `Key: Start22_1` should instead be `Major: 21, Minor: 2, ...`.


Start21_1 (from #70268):
```
// v21.1 versions. Internal versions defined here-on-forth must be even.
{
  Key:     Start21_1,
  Version: roachpb.Version{Major: 20, Minor: 2, Internal: 2},
},
```

Start 21_2:
```
{
  Key:     Start21_2,
  Version: roachpb.Version{Major: 21, Minor: 1, Internal: 1102},
},
```

Release justification: Non-production code change.
Release note: None

71856: bazel: skip recompilation when using dev+bazel r=irfansharif a=irfansharif

Fixes #71835. When switching between using `dev` and `bazel` raw, I kept
seeing our C++ protobuf dependency getting recompiled (slowly).
It appears that the bazel build for protobuf has a dependency on $PATH
(see bazelbuild/intellij#1169 and bazelbuild/bazel#7095). Specifying
[`--incompatible_strict_action_env`](https://docs.bazel.build/versions/main/command-line-reference.html#flag--incompatible_strict_action_env) pins PATH and avoids the build cache
thrashing we were seeing before.

Release note: None

Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
Co-authored-by: Celia La <celiala456@gmail.com>
Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
@celiala celiala deleted the introduce-21.2-dev-versions branch April 5, 2022 04:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants