distsqlpb,importccl: fix a regression in IMPORT #35944
Merged
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 patch ensures that the full error message (pre-unwrap) is included for errors that flow out of distsql process towards the gateway. This is important because some other internal mechanisms inside CockroachDB are expecting to see the prefixes introduced by errors.Wrap to decide other things (a bad idea, but not fixable in the short term), for example see issue cockroachdb#35942. Release note: None
|
This change is |
|
LGTM |
|
CI failure is a lint OOM. |
craig bot
pushed a commit
that referenced
this pull request
Mar 19, 2019
35912: roachtest: begin the release qualification roachtest whitelist r=nvanbenschoten a=danhhz Starting with `tpcc/nodes=3/w=max` for now so I can get the teamcity plumbing done. This test verifies that running our claimed max TPCC warehouses works. The max depends on the hardware configuration: currently 1450 on a 3-node GCE n1-standard-1 cluster or 2200 on a 3-node AWS c5d.4xlarge cluster. Release note: None 35915: sql: do not create stats on inverted index columns r=rytaft a=rytaft This commit fixes an issue in which the default columns for `CREATE STATISTICS` included inverted index columns. Since we cannot create statistics on JSON columns, running automatic statistics on a table with an inverted index resulted in the error "unable to encode table key: *tree.DJSON". This commit fixes the issue by skipping over inverted indexes when determining the default columns for `CREATE STATISTICS`. Fixes #35764 Release note (bug fix): Fixed an error that occurred when creating statistics on tables with an inverted index. 35944: distsqlpb,importccl: fix a regression in IMPORT r=RaduBerinde a=knz Fixes #35943. This patch ensures that the full error message (pre-unwrap) is included for errors that flow out of distsql process towards the gateway. This is important because some other internal mechanisms inside CockroachDB are expecting to see the prefixes introduced by errors.Wrap to decide other things (a bad idea, but not fixable in the short term), for example see issue #35942. Release note: None Co-authored-by: Daniel Harrison <[email protected]> Co-authored-by: Rebecca Taft <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]>
Build succeeded |
knz
added a commit
to knz/cockroach
that referenced
this pull request
Mar 19, 2019
Pursuant to cockroachdb#35937, cockroachdb#35944 and the discussion on cockroachdb#35854, I am realizing that there were folk under us working under two conflicting set of assumptions: - some prefer their error types unchanged, so as to be able to use type assertions to detect certain things (e.g. retry errors). - some prefer to use `errors.Wrap` or `pgerror.Wrap` liberally, so as to decorate errors with useful information to troubleshoot problems when they arise. The problem to solve is that `errors.Wrap` and alike change the type of errors. The folk who do not like this, or simply did not know about this, have historically written code that did not take this into account. I came to this realization following a comment from Andrei on issue cockroachdb#35854. Then I ran the following command: `git grep -n '[eE]rr\.(\*' pkg/sql|grep -v '_test.go:'` to find all occurrences of type casts on error objects. Then I added the missing calls to `errors.Cause()` so that the type casts become more robust again, even in the presence of more generous uses of `errors.Wrap` and `pgerror.Wrap`. Release note: None
knz
added a commit
to knz/cockroach
that referenced
this pull request
Mar 20, 2019
Pursuant to cockroachdb#35937, cockroachdb#35944 and the discussion on cockroachdb#35854, I am realizing that there were folk under us working under two conflicting set of assumptions: - some prefer their error types unchanged, so as to be able to use type assertions to detect certain things (e.g. retry errors). - some prefer to use `errors.Wrap` or `pgerror.Wrap` liberally, so as to decorate errors with useful information to troubleshoot problems when they arise. The problem to solve is that `errors.Wrap` and alike change the type of errors. The folk who do not like this, or simply did not know about this, have historically written code that did not take this into account. I came to this realization following a comment from Andrei on issue cockroachdb#35854. Then I ran the following command: `git grep -n '[eE]rr\.(\*' pkg/sql|grep -v '_test.go:'` to find all occurrences of type casts on error objects. Then I added the missing calls to `errors.Cause()` so that the type casts become more robust again, even in the presence of more generous uses of `errors.Wrap` and `pgerror.Wrap`. Release note: None
craig bot
pushed a commit
that referenced
this pull request
Mar 20, 2019
35962: sql: ensure errors are unwrapped before their type is tested r=knz a=knz Informs #35854. Pursuant to #35937, #35944 and the discussion on #35854, I am realizing that there were folk under us working under two conflicting set of assumptions: - some prefer their error types unchanged, so as to be able to use type assertions to detect certain things (e.g. retry errors). - some prefer to use `errors.Wrap` or `pgerror.Wrap` liberally, so as to decorate errors with useful information to troubleshoot problems when they arise. The problem to solve is that `errors.Wrap` and alike change the type of errors. The folk who do not like this, or simply did not know about this, have historically written code that did not take this into account. I came to this realization following a comment from Andrei on issue #35854. Then I ran the following command: `git grep -n '[eE]rr\.(\*' pkg/{sql,ccl}|grep -v '_test.go:'` to find all occurrences of type casts on error objects. Then I added the missing calls to `errors.Cause()` so that the type casts become more robust again, even in the presence of more generous uses of `errors.Wrap` and `pgerror.Wrap`. Release note: None Co-authored-by: Raphael 'kena' Poss <[email protected]>
knz
added a commit
to knz/cockroach
that referenced
this pull request
Mar 22, 2019
Pursuant to cockroachdb#35937, cockroachdb#35944 and the discussion on cockroachdb#35854, I am realizing that there were folk under us working under two conflicting set of assumptions: - some prefer their error types unchanged, so as to be able to use type assertions to detect certain things (e.g. retry errors). - some prefer to use `errors.Wrap` or `pgerror.Wrap` liberally, so as to decorate errors with useful information to troubleshoot problems when they arise. The problem to solve is that `errors.Wrap` and alike change the type of errors. The folk who do not like this, or simply did not know about this, have historically written code that did not take this into account. I came to this realization following a comment from Andrei on issue cockroachdb#35854. Then I ran the following command: `git grep -n '[eE]rr\.(\*' pkg/sql|grep -v '_test.go:'` to find all occurrences of type casts on error objects. Then I added the missing calls to `errors.Cause()` so that the type casts become more robust again, even in the presence of more generous uses of `errors.Wrap` and `pgerror.Wrap`. Release note: None
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.
Fixes #35943.
This patch ensures that the full error message (pre-unwrap) is
included for errors that flow out of distsql process towards the
gateway.
This is important because some other internal mechanisms inside
CockroachDB are expecting to see the prefixes introduced by
errors.Wrap to decide other things (a bad idea, but not fixable in the
short term), for example see issue #35942.
Release note: None