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.

Sign up for GitHub

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

Jump to bottom

sql, *: simplify remove dependencies on pgerror, util/log #37813

Merged
merged 6 commits into from
Jun 18, 2019
Merged

sql, *: simplify remove dependencies on pgerror, util/log #37813

merged 6 commits into from
Jun 18, 2019

Conversation

knz
Copy link
Contributor

@knz knz commented May 25, 2019 •
edited
Loading

These changes are mostly mechanical (I used grep and perl, followed by crlfmt).

This compacts the source code: overall -300 locs, and shorter lines.

@knz knz requested review from jordanlewis, RaduBerinde and a team May 25, 2019 11:54
@knz knz requested a review from a team as a code owner May 25, 2019 11:54
@knz knz requested review from a team May 25, 2019 11:54
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz force-pushed the 20190523-errlib-cleanup-simplify branch 3 times, most recently from 62ecc5e to 082ad10 Compare May 25, 2019 13:12
@knz knz force-pushed the 20190523-errlib-diffs branch from eddb962 to 53e49e4 Compare June 4, 2019 08:51
@knz knz force-pushed the 20190523-errlib-cleanup-simplify branch from 082ad10 to 39d319a Compare June 4, 2019 08:51
@knz knz requested a review from andreimatei June 4, 2019 09:02
@knz knz force-pushed the 20190523-errlib-diffs branch from 53e49e4 to d93ccf7 Compare June 4, 2019 09:47
@knz knz force-pushed the 20190523-errlib-cleanup-simplify branch from 39d319a to d406ed0 Compare June 4, 2019 09:47
@knz knz force-pushed the 20190523-errlib-diffs branch from d93ccf7 to 5fe37e7 Compare June 4, 2019 10:16
@knz knz force-pushed the 20190523-errlib-cleanup-simplify branch 2 times, most recently from f9c340a to 13559ad Compare June 4, 2019 10:55
@andreimatei
Copy link
Contributor

LGTM to everything but the last commit; I'm happy to see these changes.
What's the benefit of that one - extracting the error codes into their own package?

@knz
Copy link
Contributor Author

knz commented Jun 4, 2019

What's the benefit of that one - extracting the error codes into their own package?

Avoid a cyclic dependency (as usual)

@knz
Copy link
Contributor Author

knz commented Jun 4, 2019

Also it's more readable: "pgcode.Foo" is shorter than "pgwire.CodeFoo"

@andreimatei
Copy link
Contributor

Do you remember what the dependency you were breaking was?

In any case, LGTM

@knz knz force-pushed the 20190523-errlib-diffs branch from 5fe37e7 to 3b70f6d Compare June 5, 2019 16:04
@knz knz force-pushed the 20190523-errlib-cleanup-simplify branch from 13559ad to 8095471 Compare June 5, 2019 16:05
@knz knz force-pushed the 20190523-errlib-diffs branch from bf152aa to ef803ab Compare June 11, 2019 08:14
@knz knz force-pushed the 20190523-errlib-cleanup-simplify branch from 02d7cc4 to 4f93946 Compare June 11, 2019 08:14
@knz knz force-pushed the 20190523-errlib-diffs branch from ef803ab to 8e58782 Compare June 18, 2019 12:07
@knz knz force-pushed the 20190523-errlib-cleanup-simplify branch from 4f93946 to 43b2d5e Compare June 18, 2019 12:07
This was referenced Jun 18, 2019
Merged
Merged
@knz knz force-pushed the 20190523-errlib-diffs branch from 8e58782 to ae7ade0 Compare June 18, 2019 12:22
@knz knz force-pushed the 20190523-errlib-cleanup-simplify branch from 43b2d5e to 6f21e35 Compare June 18, 2019 12:23
@knz knz force-pushed the 20190523-errlib-diffs branch from ae7ade0 to 511f01f Compare June 18, 2019 12:56
@knz knz force-pushed the 20190523-errlib-cleanup-simplify branch from 6f21e35 to f557ea1 Compare June 18, 2019 12:56
@knz knz force-pushed the 20190523-errlib-diffs branch from 511f01f to dc2b1c1 Compare June 18, 2019 13:04
@knz knz force-pushed the 20190523-errlib-cleanup-simplify branch from f557ea1 to 134b2da Compare June 18, 2019 13:04
@knz knz force-pushed the 20190523-errlib-diffs branch from dc2b1c1 to 7c3c81a Compare June 18, 2019 13:52
@knz knz force-pushed the 20190523-errlib-cleanup-simplify branch from 134b2da to 9766577 Compare June 18, 2019 13:52
@knz knz force-pushed the 20190523-errlib-diffs branch from 7c3c81a to 684b49b Compare June 18, 2019 14:25
@knz knz force-pushed the 20190523-errlib-cleanup-simplify branch from 9766577 to 027161f Compare June 18, 2019 14:25
@knz knz requested a review from a team June 18, 2019 15:38
@knz knz force-pushed the 20190523-errlib-cleanup-simplify branch from b47b2b1 to 516c985 Compare June 18, 2019 15:41
knz added 6 commits June 18, 2019 17:43
@knz
sql,ccl: avoid using CodeDataException
e431c92 
A while ago I had changed many calls to
`errors.Errorf`/`errors.New`/`fmt.Errorf`/`errors.Wrap` to use
`pgerror` error constructors instead. The goal was to ensure the error
objects were decorated with safe details suitable for reporting.

However using those APIs also required a pg error code, even in cases
where none was obviously available. So I chose then to use the pg code
"CodeDataException" as a dummy value.

Now that the `errors` library is able to annotate errors in all
the good ways, the `pgerror` interface is not needed any more.
This commit reverts the past change.

This change is nearly all mechanical, using perl.
The only manual change is to the function `wrapRowErr` in the
`importccl` package.

Release note: None
@knz
sql,*: use error constructors from the errors package directly
f1a8f4e 
... instead of the aliased definitions in `pgerror`.

(This change is entirely mechanical, using perl)

Release note: None
@knz
sql,ccl: avoid errors.Wrap with an empty string
caf2fce 
Release note: None
@knz
sql,ccl: use errors.Safe instead of log.Safe
a77e02b 
... where appropriate. This drops the dependency on `util/log` in many
cases.

(This change was mechanical, using perl)

Release note: None
@knz
pgwire: use pgcode.XXXX instead of pgerror.CodeXXXError
eff124f 
... and remove pgwire/pgerror/codes.go

Release note: None
@knz
build: run testrace on packages one after the other
6cd22cc 
Previously `teamcity-testrace.sh` would run `testrace` passing all the
modified packages simultaneously in the PKG make var. This causes `go
test` to issue all the tests concurrently, regardless of available
hardware. If there are sufficiently many packages modified, this
overloads the machines, makes test run much slower than usual, and
triggers bad behavior (timeouts).

This patch alleviates the potential problem by running the tests one
after the other.

Release note: None
@knz knz force-pushed the 20190523-errlib-diffs branch from 684b49b to dc13807 Compare June 18, 2019 15:45
@knz knz force-pushed the 20190523-errlib-cleanup-simplify branch from 516c985 to 6cd22cc Compare June 18, 2019 15:45
craig bot pushed a commit that referenced this pull request Jun 18, 2019
@knz
Merge #37121
999953e 
37121: errors: introduce a general-purpose modular error library with good properties r=knz a=knz

This PR introduces an error library implemented after the principles laid out in #36987.

It subsumes the following two PRs:

- #38127 *: demonstrate uses of the errors library 
  - review approval: #38127 (comment)
- #37813 sql, *: simplify remove dependencies on pgerror, util/log
  - review approval: #37813 (comment)

Release note: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>
@knz knz merged commit 20d47d7 into 20190523-errlib-diffs Jun 18, 2019
@knz knz deleted the 20190523-errlib-cleanup-simplify branch June 18, 2019 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jordanlewis jordanlewis Awaiting requested review from jordanlewis

@RaduBerinde RaduBerinde Awaiting requested review from RaduBerinde

@andreimatei andreimatei Awaiting requested review from andreimatei

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@knz @cockroach-teamcity @andreimatei