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

sql: add SET CLUSTER SETTING wrapper to alter system.settings #14691

Merged
merged 4 commits into from
Apr 12, 2017

Conversation

dt
Copy link
Member

@dt dt commented Apr 7, 2017

Currently a very thin wrapper around just upserting strings, just to get the various boilerplate and plumbing in place.

@dt dt requested review from bdarnell, benesch and tamird April 7, 2017 01:43
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@dt dt force-pushed the set branch 2 times, most recently from 6e1d578 to de841af Compare April 7, 2017 12:36
@tamird
Copy link
Contributor

tamird commented Apr 10, 2017

Reviewed 1 of 1 files at r1, 5 of 5 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


pkg/sql/set.go, line 104 at r3 (raw file):

			ctx, "update-setting", p.txn, "DELETE FROM system.settings WHERE name = $1", name,
		)
		return &emptyNode{}, err

nit: return here only if err is non-nil, and then return nil at the bottom?


pkg/sql/set.go, line 106 at r3 (raw file):

		return &emptyNode{}, err
	case 1:
		// TODO(dt): validate and properly encode str according to type.

seems important enough to do now


pkg/sql/set.go, line 122 at r3 (raw file):

func (p *planner) toSettingString(raw parser.Expr) (string, error) {
	// TODO(dt): typecheck and handle according to setting's desired type.

ditto


Comments from Reviewable

@dt
Copy link
Member Author

dt commented Apr 10, 2017

Review status: 1 of 8 files reviewed at latest revision, 3 unresolved discussions.


pkg/sql/set.go, line 104 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

nit: return here only if err is non-nil, and then return nil at the bottom?

Done.


pkg/sql/set.go, line 106 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

seems important enough to do now

i've got it queued up locally for a fast follow, but obviously it depends on the registry in #14688, to lookup the types to do the validation, plus I wanted to keep individual changes a manageable size.


pkg/sql/set.go, line 122 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

ditto

ditto.


Comments from Reviewable

@tamird
Copy link
Contributor

tamird commented Apr 10, 2017

Reviewed 7 of 7 files at r4, 5 of 5 files at r5, 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


pkg/sql/set.go, line 106 at r3 (raw file):

Previously, dt (David Taylor) wrote…

i've got it queued up locally for a fast follow, but obviously it depends on the registry in #14688, to lookup the types to do the validation, plus I wanted to keep individual changes a manageable size.

Understood, and yet I think it should go with this =/


pkg/sql/set.go, line 113 at r6 (raw file):

		}
		upsertQ := "UPSERT INTO system.settings (name, value, lastUpdated, valueType) VALUES ($1, $2, NOW(), $3)"
		if _, err = ie.ExecuteStatementInTransaction(

:=


Comments from Reviewable

dt added 3 commits April 11, 2017 20:28
Currently a very thin wrapper around just upserting strings, just to get the various boilerplate and plumbing in place.
@dt
Copy link
Member Author

dt commented Apr 11, 2017

Review status: 1 of 9 files reviewed at latest revision, 3 unresolved discussions.


pkg/sql/set.go, line 106 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Understood, and yet I think it should go with this =/

Done.


Comments from Reviewable

@dt
Copy link
Member Author

dt commented Apr 11, 2017

Review status: 1 of 9 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


pkg/sql/set.go, line 113 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

:=

Done.


Comments from Reviewable

@tamird
Copy link
Contributor

tamird commented Apr 12, 2017

:lgtm:


Reviewed 7 of 7 files at r7, 5 of 5 files at r8, 2 of 2 files at r9, 3 of 3 files at r10.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


pkg/sql/set.go, line 99 at r10 (raw file):

	typ, ok := settings.TypeOf(name)
	if !ok {
		return nil, errors.Errorf("unknown cluster settings %q", name)

setting

'%s'


pkg/sql/set.go, line 142 at r10 (raw file):

			return "", err
		}
		encoded, err := f(d)

return f(d)?


Comments from Reviewable

@dt
Copy link
Member Author

dt commented Apr 12, 2017

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


pkg/sql/set.go, line 99 at r10 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

setting

'%s'

Done.


pkg/sql/set.go, line 142 at r10 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

return f(d)?

Done.


Comments from Reviewable

@dt
Copy link
Member Author

dt commented Apr 12, 2017

TFTR!


Review status: 8 of 9 files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@tamird
Copy link
Contributor

tamird commented Apr 12, 2017

Reviewed 1 of 1 files at r11.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@dt dt merged commit 1638d6a into cockroachdb:master Apr 12, 2017
@dt dt deleted the set branch May 31, 2017 13:23
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.

3 participants