roachpb: remove SetInner in favor of MustSetInner #56600
Merged
+28
−50
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
tbg
force-pushed
the
must-error-detail
branch
from
4107a83 to
b5085d2
Compare
nvanbenschoten
approved these changes
Nov 12, 2020
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @tbg)
pkg/roachpb/batch_generated.go, line 268 at r1 (raw file):
// MustSetInner sets the error in the union. func (ru *ErrorDetail) MustSetInner(r error) { ru.Reset()
Looks like the spacing is a little off here. Is there a diff on this file if you run it through gofmt?
tbg
force-pushed
the
must-error-detail
branch
from
b5085d2 to
e121a9b
Compare
tbg
commented
Nov 12, 2020
There was a problem hiding this comment.
TFTR!
bors r=nvanbenschoten
Reviewable status:
pkg/roachpb/batch_generated.go, line 268 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Looks like the spacing is a little off here. Is there a diff on this file if you run it through gofmt?
fixed
craig bot
pushed a commit
that referenced
this pull request
Nov 12, 2020
55325: ui: Transactions link between Sessions and Statements r=dhartunian a=elkmaster Resolves: #55131, #56244 Release note (admin ui change): Link to the Transactions page is now shown between the Sessions and Statements links in the left hand navigation. This more clearly reflects the hierarchy between the 3 concepts. 56346: testcluster: minor logging improvements r=andreimatei a=andreimatei Log when TestCluster quiescing starts, and add a node log tag to each node's quiescing ctx so messages from different nodes can be disambiguated. Release note: None 56437: cli, ui: dismiss release notes signup banner per environment variable r=knz,dhartunian a=nkodali Previously, the signup banner could only be dismissed manually. For internal testing purposes, this banner is unnecessary. This change provides a way to dismiss the signup banner upon start of a cluster via the cli by setting the environment variable COCKROACH_UI_RELEASE_NOTES_SIGNUP_DISMISSED=true. Resolves #46998 Release note: none 56533: backupccl: add feature flag support for BACKUP, RESTORE r=otan a=angelapwen Follow-up to the RFC at #55778. This addresses the SRE use case mentioned in #51643 — instead of moving forward with a global denylist as the RFC indicated, we are prototyping feature flags via cluster settings to turn on/off requested features. The first part of the prototype will be done on `BACKUP` and `RESTORE` commands. See [this doc](https://docs.google.com/document/d/1nZSdcK7YprL0P4TAuseY-mvlYnd82IaJ_ptAQDoWB6o/edit?) for further details. Note that the logic test under `ccl/backupccl/testdata/backup-restore/feature-flags` can be tested with the command `make test PKG=./pkg/ccl/backupccl TESTS='TestBackupRestoreDataDriven'` — Commit message below — Adds a cluster setting to toggle a feature flag for the BACKUP and RESTORE commands off and on; as well as a broad category for Bulk IO commands. Currently disabling the cluster setting for Bulk IO will only disable BACKUP and RESTORE jobs, but other types may be included in this category in the future.. The feature is being introduced to address a Cockroach Cloud SRE use case: needing to disable certain categories of features in case of cluster failure. Release note (enterprise change): Adds cluster settings to enable/ disable the BACKUP and RESTORE commands. If a user attempts to use these features while they are disabled, an error indicating that the database administrator has disabled the feature is surfaced. Example usage for the database administrator: SET CLUSTER SETTING feature.bulkio.backup.enabled = FALSE; SET CLUSTER SETTING feature.bulkio.backup.enabled = TRUE; SET CLUSTER SETTING feature.bulkio.restore.enabled = FALSE; SET CLUSTER SETTING feature.bulkio.restore.enabled = TRUE; SET CLUSTER SETTING feature.bulkio.enabled = FALSE; SET CLUSTER SETTING feature.bulkio.enabled = TRUE; 56591: ui: fix Overview screen in OSS builds r=dhartunian a=davepacheco Previously, when using OSS builds (created with `make buildoss`), when you loading the DB Console in your browser, you'd get "Page Not Found". The route for the overview page was missing the leading '/'. This bug appears to have been introduced in 722c932. Release note (admin ui change): This fixes a bug where users of the OSS builds of CockroachDB would see "Page Not Found" when loading the Console. 56600: roachpb: remove SetInner in favor of MustSetInner r=nvanbenschoten a=tbg As of a recent commit, `ErrorDetail.SetInner` became unused, and we can switch to a `MustSetInner` pattern for `ErrorDetail`. Since the codegen involved is shared with {Request,Response}Union, those lose the `SetInner` setter as well; we were always asserting on the returned bool there anyway so this isn't changing anything. Release note: None Co-authored-by: Vlad Los <[email protected]> Co-authored-by: Andrei Matei <[email protected]> Co-authored-by: Namrata Kodali <[email protected]> Co-authored-by: angelapwen <[email protected]> Co-authored-by: Joshua M. Clulow <[email protected]> Co-authored-by: Tobias Grieger <[email protected]>
|
Build failed (retrying...): |
|
lint failing :( bors r- |
|
Canceled. |
As of a recent commit, `ErrorDetail.SetInner` became unused, and
we can switch to a `MustSetInner` pattern for `ErrorDetail`. Since
the codegen involved is shared with {Request,Response}Union, those
lose the `SetInner` setter as well; we were always asserting on
the returned bool there anyway so this isn't changing anything.
Release note: None
|
Thanks @andreimatei! Third time's the charm. bors r=nvanbenschoten |
tbg
force-pushed
the
must-error-detail
branch
from
e121a9b to
ab0b87a
Compare
|
🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As of a recent commit,
ErrorDetail.SetInnerbecame unused, andwe can switch to a
MustSetInnerpattern forErrorDetail. Sincethe codegen involved is shared with {Request,Response}Union, those
lose the
SetInnersetter as well; we were always asserting onthe returned bool there anyway so this isn't changing anything.
Release note: None