sql, distsql: add enum cluster settings, add distsql cluster setting #15307

rjnn · 2017-04-24T20:35:08Z

@knz: commit 1.
@andreimatei: commit 2. Please note the TODO(DONOTMERGE), help will be appreciated with fixing that! Thank you.

cockroach-teamcity · 2017-04-24T20:35:16Z

This change is

andreimatei · 2017-04-24T21:16:41Z

Review status: 0 of 10 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

pkg/settings/registry.go, line 271 at r1 (raw file):

	enumValues   map[string]int64
	defaultValue int64
	v            int64

nit: v doesn't seem like a descriptive name

pkg/settings/registry.go, line 283 at r1 (raw file):

func (e *EnumSetting) Typ() string {
	return "e"

nit: "e"? "enum"?

pkg/settings/registry.go, line 294 at r1 (raw file):

	v, ok := e.enumValues[strings.ToLower(k)]
	if !ok {
		return errors.Errorf("cannot set cluster setting to value '%s', acceptable values are %s", k, enumValuesToDesc(e.enumValues))

s/%s/%q

pkg/settings/registry.go, line 308 at r1 (raw file):

	buffer.WriteString("[")
	for k, v := range enumValues {
		buffer.WriteString(fmt.Sprintf("(%s:%d)", strings.ToLower(k), v))

why include the values in this string? Can they be of interest to a client? At least in the "acceptable values" error msg they seem out of place.

pkg/settings/registry.go, line 424 at r1 (raw file):

}

var _ Setting = &BoolSetting{}

nit: put these assertions next to the definition of the structs. Otherwise a new implementation copying an existing one will not see it.

pkg/settings/registry.go, line 315 at r2 (raw file):

// RegisterIntSetting defines a new setting with type int.
func RegisterEnumSetting(key, desc string, defVal int64, enumValues map[string]int64) *EnumSetting {

I think it'd be nicer if the default was passed as the key (string), not the value.

pkg/settings/registry.go, line 315 at r2 (raw file):

// RegisterIntSetting defines a new setting with type int.
func RegisterEnumSetting(key, desc string, defVal int64, enumValues map[string]int64) *EnumSetting {

nit: the name "defVal" for a default is not used outside of this file. It's called "defaultValue" in the struct, which I prefer. Also the next argument spells out "Values".

pkg/sql/logic_test.go, line 1443 at r2 (raw file):

		var cleanupFuncs []func()
		if cfg.defaultDistSQLMode != "" {
			// TODO(DONOTMERGE): how do we run a SET CLUSTER SETTING distsql.mode = on SQL command here?

well you can't run it here cause the cluster doesn't exist yet, right? It's only started in the setupAndRunFile below I think. So I guess you'll have to defer this initialization.
Btw, are these "SET CLUSTER SETTING" statements synchronous in any way? Or you'll have to wait for propagation through the cluster in another way?

pkg/sql/session.go, line 109 at r2 (raw file):

	"sql.defaults.distsql",
	"Default distributed SQL execution mode",
	1,

nit: what's 1? Consider commenting the arg inline.

pkg/sql/session.go, line 109 at r2 (raw file):

	"sql.defaults.distsql",
	"Default distributed SQL execution mode",
	1,

if this is the default and means "auto", we're not ready yet.

pkg/sql/session.go, line 114 at r2 (raw file):

		"Auto":   1,
		"On":     2,
		"Always": -1,

-1? And you parse any negative value as "always"?

Use the actualy consts (cast to int) here, and cast the other way in distSQLExecModeFromInt.

Comments from Reviewable

RaduBerinde · 2017-04-25T03:47:50Z

Review status: 0 of 10 files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

pkg/settings/registry.go, line 269 at r1 (raw file):

// EnumSetting is a StringSetting that restricts the values to be one of the `enumValues`
type EnumSetting struct {
	enumValues   map[string]int64

why int64 (and not just int)

pkg/settings/registry.go, line 328 at r1 (raw file):

}

// TestingIntSetting returns a mock, unregistered enum setting for testing.

TestingEnumSetting. Also is this actually used anywhere?

pkg/sql/logic_test.go, line 1443 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

well you can't run it here cause the cluster doesn't exist yet, right? It's only started in the setupAndRunFile below I think. So I guess you'll have to defer this initialization.
Btw, are these "SET CLUSTER SETTING" statements synchronous in any way? Or you'll have to wait for propagation through the cluster in another way?

Seems to me that we need some kind of TestingKnob that allows you to override the default values of cluster settings.

pkg/sql/session.go, line 111 at r2 (raw file):

	1,
	map[string]int64{
		"Off":    0,

why do we have a completely new mapping instead of using the actual enum values (distSQLOff etc)?

pkg/sql/session.go, line 114 at r2 (raw file):

		"Auto":   1,
		"On":     2,
		"Always": -1,

I don't think we want Always as a possibility for the cluster setting.

Comments from Reviewable

dt · 2017-04-25T04:14:46Z

Review status: 0 of 10 files reviewed at latest revision, 19 unresolved discussions, some commit checks failed.

pkg/server/settingsworker.go, line 101 at r1 (raw file):

		}

		return u.Set(k, v, t)

nope, it's a bit subtle, but as you'll see from the test failures, an error from Set is a log-and-continue situation.

pkg/settings/registry.go, line 268 at r1 (raw file):

// EnumSetting is a StringSetting that restricts the values to be one of the `enumValues`
type EnumSetting struct {

I think you can just embed an existing Setting impl and only override the methods as needed, similar to ByteSizeSetting.

alternately, you could just add an optional whitelist or validate param to the existing StringSetting?

pkg/settings/registry.go, line 269 at r1 (raw file):

Previously, RaduBerinde wrote…

why int64 (and not just int)

since we're using atomic and have to specify the size of our integers anyway, intsettings just use int64 thoughout rather than going back and forth to a platform-defined int (since their callers often are using int64 too).

pkg/settings/registry.go, line 279 at r1 (raw file):

func (e *EnumSetting) String() string {
	return EncodeInt(e.Get())

String() is supposed to print for humans, so I'd rather return the string, not the int.

pkg/settings/registry.go, line 283 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: "e"? "enum"?

nope, single char is in keeping with the rest (and leaves using a char to store them open).

pkg/settings/registry.go, line 294 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

s/%s/%q

@tamird has been requesting the opposite in all my recent reviews, which is where the error copy/pasted here came from.

pkg/settings/registry.go, line 315 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I think it'd be nicer if the default was passed as the key (string), not the value.

strong ditto

pkg/settings/registry.go, line 315 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: the name "defVal" for a default is not used outside of this file. It's called "defaultValue" in the struct, which I prefer. Also the next argument spells out "Values".

defVal is consistent with the others @knz added, so unless you're gonna rename them all, consistency wins.

pkg/settings/updater.go, line 98 at r1 (raw file):

		setting.set(d)
	case *EnumSetting:
		err := setting.set(rawValue)

just return setting.set(rawValue)

also, consider mirroring pattern of numericSetting and just combining with the existing StringSetting's case.

importantly though, in the event that the settings table contains a value this node thinks is invalid, you want to keep using the value this node was using? fall back to default?

pkg/sql/logic_test.go, line 1443 at r2 (raw file):

Previously, RaduBerinde wrote…

Seems to me that we need some kind of TestingKnob that allows you to override the default values of cluster settings.

that's what the TestingFoo mocks are for -- you swap the pkg var with one.

Comments from Reviewable

knz · 2017-04-25T09:42:51Z

Too many cooks in this game. :-)

I suggest we stick to reviewing the two commits separately. Andrei, Arjun is following pattern in this code, if you think the settings package deserves some clean-up, please file a follow-up clean-up issue.

Reviewed 6 of 6 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 21 unresolved discussions, some commit checks failed.

pkg/settings/registry.go, line 271 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: v doesn't seem like a descriptive name

It's following pattern.

pkg/settings/registry.go, line 279 at r1 (raw file):

Previously, dt (David Taylor) wrote…

String() is supposed to print for humans, so I'd rather return the string, not the int.

We discussed this earlier with Arjun -- the setting description will list the mapping, and String() can print the value. This allows us to change the symbolic names later for documentation purposes without having to re-interpret whatever debug zip or the admin RPC reports (and both use String())
(I do not feel strongly about this though)

pkg/settings/registry.go, line 294 at r1 (raw file):

Previously, dt (David Taylor) wrote…

@tamird has been requesting the opposite in all my recent reviews, which is where the error copy/pasted here came from.

Arjun: please also allow the user to set enums using the numeric value.

pkg/settings/registry.go, line 306 at r1 (raw file):

func enumValuesToDesc(enumValues map[string]int64) string {
	var buffer bytes.Buffer
	buffer.WriteString("[")

This printout is ... not very user-friendly. Try this:

buffer.WriteByte('(')
i := 0
for k, v := range enumValues {
   if i > 0 {
       buffer.WriteString(", ")
   }
   fmt.Fprintf(&buffer, "%d = %s", v, k)
}
buffer.WriteByte(')')

pkg/settings/registry.go, line 308 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

why include the values in this string? Can they be of interest to a client? At least in the "acceptable values" error msg they seem out of place.

My request: we want to be able to change the symbolic names for documentation reasons; the mapping in the description explains what's going on if a name changes.

pkg/settings/registry.go, line 315 at r1 (raw file):

// RegisterIntSetting defines a new setting with type int.
func RegisterEnumSetting(key, desc string, defVal int64, enumValues map[string]int64) *EnumSetting {

Please add an assert (with panic) that the map is not empty and that the default value is in the map.

pkg/settings/registry.go, line 315 at r2 (raw file):

Previously, dt (David Taylor) wrote…

strong ditto

yeah default must be string

Comments from Reviewable

knz · 2017-04-25T09:43:24Z

Review status: all files reviewed at latest revision, 21 unresolved discussions, some commit checks failed.

pkg/settings/registry.go, line 306 at r1 (raw file):

Previously, knz (kena) wrote…

This printout is ... not very user-friendly. Try this:

buffer.WriteByte('(')
i := 0
for k, v := range enumValues {
   if i > 0 {
       buffer.WriteString(", ")
   }
   fmt.Fprintf(&buffer, "%d = %s", v, k)
}
buffer.WriteByte(')')

there's a i++ missing my code sample above.

Comments from Reviewable

rjnn · 2017-04-25T14:36:34Z

pkg/server/settingsworker.go, line 101 at r1 (raw file):

Previously, dt (David Taylor) wrote…

nope, it's a bit subtle, but as you'll see from the test failures, an error from Set is a log-and-continue situation.

That sucks. We really want it to fail when someone says SET CLUSTER SETTING foo = notAValidEnum rather than returning SET and silently failing. Is there a way to achieve that?

Comments from Reviewable

knz · 2017-04-25T14:39:54Z

Review status: all files reviewed at latest revision, 21 unresolved discussions, some commit checks failed.

pkg/server/settingsworker.go, line 101 at r1 (raw file):

Previously, arjunravinarayan (Arjun Narayan) wrote…

That sucks. We really want it to fail when someone says SET CLUSTER SETTING foo = notAValidEnum rather than returning SET and silently failing. Is there a way to achieve that?

You can extend toSettingString() for that.

Comments from Reviewable

dt · 2017-04-25T14:44:16Z

Review status: all files reviewed at latest revision, 21 unresolved discussions, some commit checks failed.

pkg/server/settingsworker.go, line 101 at r1 (raw file):

Previously, knz (kena) wrote…

You can extend toSettingString() for that.

So this wouldn't do that for you anyway -- this code is what loads the table data from gossip to the in-memory values on each node. By the time a valueis in gossip, the SET a user ran has committed (elsewhere) long ago.

As knz says, you want to do validation in set.go has it services the SET stmt.

Comments from Reviewable

andreimatei · 2017-04-25T17:04:09Z

I realize the "following the pattern", but patterns are useful where they help readability or something, not patterns for patterns sake. For example, if you agree that defaultValue is better than defVal, then this change, even done only in one place, only serves to improve the readability of all the structs in the package (if you have happened to see the defaultValue before seeing defVal). In any case, those are all nits on my part, not to be confused with important comments.

Review status: all files reviewed at latest revision, 21 unresolved discussions, some commit checks failed.

Comments from Reviewable

andreimatei · 2017-04-25T17:53:14Z

Review status: all files reviewed at latest revision, 21 unresolved discussions, some commit checks failed.

pkg/settings/registry.go, line 294 at r1 (raw file):

Previously, knz (kena) wrote…

Arjun: please also allow the user to set enums using the numeric value.

Why's tamir down on %q?

Comments from Reviewable

tamird · 2017-04-25T17:56:03Z

The meaning is slightly different. From https://golang.org/pkg/fmt/: %s the uninterpreted bytes of the string or slice %q a double-quoted string safely escaped with Go syntax

…

On Tue, Apr 25, 2017 at 1:53 PM, Andrei Matei ***@***.***> wrote: Review status: all files reviewed at latest revision, 21 unresolved discussions, some commit checks failed. ------------------------------ *pkg/settings/registry.go, line 294 at r1 <https://reviewable.io:443/reviews/cockroachdb/cockroach/15307#-KiWhPvtf5lmnuCHGy5B:-KiaEcajufixPwRzuSAS:b-f25c46> (raw file <https://github.com/cockroachdb/cockroach/blob/fc743e63315990f08181d0ef1fe62bc2b714e1e7/pkg/settings/registry.go#L294>):* *Previously, knz (kena) wrote…* Arjun: please also allow the user to set enums using the numeric value. Why's tamir down on %q? ------------------------------ *Comments from Reviewable <https://reviewable.io:443/reviews/cockroachdb/cockroach/15307>* — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15307 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsPPHiVMkxiSToWUOlZ5Ca0xZK9DIlks5rzjMPgaJpZM4NGrYP> .

tamird · 2017-04-25T17:56:49Z

Maybe %q is the right thing after all ¯\_(ツ)_/¯ On Tue, Apr 25, 2017 at 1:55 PM, Tamir Duberstein <[email protected]> wrote:

…

The meaning is slightly different. From https://golang.org/pkg/fmt/: %s the uninterpreted bytes of the string or slice %q a double-quoted string safely escaped with Go syntax On Tue, Apr 25, 2017 at 1:53 PM, Andrei Matei ***@***.***> wrote: > Review status: all files reviewed at latest revision, 21 unresolved > discussions, some commit checks failed. > ------------------------------ > > *pkg/settings/registry.go, line 294 at r1 > <https://reviewable.io:443/reviews/cockroachdb/cockroach/15307#-KiWhPvtf5lmnuCHGy5B:-KiaEcajufixPwRzuSAS:b-f25c46> > (raw file > <https://github.com/cockroachdb/cockroach/blob/fc743e63315990f08181d0ef1fe62bc2b714e1e7/pkg/settings/registry.go#L294>):* > *Previously, knz (kena) wrote…* > > Arjun: please also allow the user to set enums using the numeric value. > > Why's tamir down on %q? > ------------------------------ > > *Comments from Reviewable > <https://reviewable.io:443/reviews/cockroachdb/cockroach/15307>* > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#15307 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABdsPPHiVMkxiSToWUOlZ5Ca0xZK9DIlks5rzjMPgaJpZM4NGrYP> > . >

rjnn · 2017-04-25T21:58:21Z

Switched everything to defaultValue to keep you happy.

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

pkg/settings/registry.go, line 268 at r1 (raw file):

Previously, dt (David Taylor) wrote…

I think you can just embed an existing Setting impl and only override the methods as needed, similar to ByteSizeSetting.

alternately, you could just add an optional whitelist or validate param to the existing StringSetting?

Done, using IntSetting as the base, since we want to print integers for the String() function.

pkg/settings/registry.go, line 279 at r1 (raw file):

Previously, knz (kena) wrote…

We discussed this earlier with Arjun -- the setting description will list the mapping, and String() can print the value. This allows us to change the symbolic names later for documentation purposes without having to re-interpret whatever debug zip or the admin RPC reports (and both use String())
(I do not feel strongly about this though)

I was with @dt, but I'm convinced by the point that allowing names to be changed easily is worth it.

pkg/settings/registry.go, line 294 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Why's tamir down on %q?

As a side note, I don't really like this nitpicking about using %s vs %q etc in error messages. If this is something that you care about so strongly, you should make crlfmt do it. I don't mind suggestions as in this case, but it's crazy that multiple people feel strongly about things like this, and in opposite directions in different parts of the company. You can already see the bikeshedding in this issue because it crosses package boundaries about various conventions. Thanks very much to @knz for pushing back on that.

@knz I don't know how to do that, lets talk about it.

pkg/settings/registry.go, line 306 at r1 (raw file):

Previously, knz (kena) wrote…

there's a i++ missing my code sample above.

Done.

pkg/settings/registry.go, line 315 at r1 (raw file):

Previously, knz (kena) wrote…

Please add an assert (with panic) that the map is not empty and that the default value is in the map.

Done.

pkg/settings/registry.go, line 328 at r1 (raw file):

Previously, RaduBerinde wrote…

TestingEnumSetting. Also is this actually used anywhere?

It is now, in the logic tests (but good catch).

pkg/settings/registry.go, line 424 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: put these assertions next to the definition of the structs. Otherwise a new implementation copying an existing one will not see it.

Done.

pkg/settings/registry.go, line 315 at r2 (raw file):

Previously, knz (kena) wrote…

yeah default must be string

Done.

pkg/settings/updater.go, line 98 at r1 (raw file):

Previously, dt (David Taylor) wrote…

just return setting.set(rawValue)

also, consider mirroring pattern of numericSetting and just combining with the existing StringSetting's case.

importantly though, in the event that the settings table contains a value this node thinks is invalid, you want to keep using the value this node was using? fall back to default?

Fixed to just returning setting.set. Yes, there's no real good way to handle that case, I think just keep using the value it was using is fine.

pkg/sql/logic_test.go, line 1443 at r2 (raw file):

Previously, dt (David Taylor) wrote…

that's what the TestingFoo mocks are for -- you swap the pkg var with one.

I still don't know how to do this, would appreciate a little help. I'll come find you @dt.

pkg/sql/session.go, line 109 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: what's 1? Consider commenting the arg inline.

nit: use an editor that does this for you! Anyway this is a moo point since we now pass default values by string.

pkg/sql/session.go, line 109 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

if this is the default and means "auto", we're not ready yet.

Done.

pkg/sql/session.go, line 111 at r2 (raw file):

Previously, RaduBerinde wrote…

why do we have a completely new mapping instead of using the actual enum values (distSQLOff etc)?

Fixed.

pkg/sql/session.go, line 114 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

-1? And you parse any negative value as "always"?

Use the actualy consts (cast to int) here, and cast the other way in distSQLExecModeFromInt.

Fixed.

pkg/sql/session.go, line 114 at r2 (raw file):

Previously, RaduBerinde wrote…

I don't think we want Always as a possibility for the cluster setting.

Removed.

pkg/server/settingsworker.go, line 101 at r1 (raw file):

Previously, dt (David Taylor) wrote…

So this wouldn't do that for you anyway -- this code is what loads the table data from gossip to the in-memory values on each node. By the time a valueis in gossip, the SET a user ran has committed (elsewhere) long ago.

As knz says, you want to do validation in set.go has it services the SET stmt.

I'll add this and add a test to ensure that the SET command fails on an invalid settings change.

Comments from Reviewable

knz · 2017-04-25T22:14:35Z

Reviewed 5 of 8 files at r3, 4 of 4 files at r4.
Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

pkg/settings/registry.go, line 294 at r1 (raw file):

Previously, arjunravinarayan (Arjun Narayan) wrote…

As a side note, I don't really like this nitpicking about using %s vs %q etc in error messages. If this is something that you care about so strongly, you should make crlfmt do it. I don't mind suggestions as in this case, but it's crazy that multiple people feel strongly about things like this, and in opposite directions in different parts of the company. You can already see the bikeshedding in this issue because it crosses package boundaries about various conventions. Thanks very much to @knz for pushing back on that.

@knz I don't know how to do that, lets talk about it.

if !ok {
    // Try to see if there is a valid numeric value instead
   var err error
   v, err = strconv.ParseInt(k, 10, 64)
   if err != nil {
       return invalidEnumValueError(e, k)
   }
   isValid := false
   for _, allowedValue := range e.enumValues {
        if v == allowedValue {
           isValid = true
           break
        }
   }
   if !isValid {
       return invalidEnumValueError(e, k)
   }
}
atomic.StoreInt64(&e.v, v)

pkg/sql/logic_test.go, line 1443 at r2 (raw file):

Previously, arjunravinarayan (Arjun Narayan) wrote…

I still don't know how to do this, would appreciate a little help. I'll come find you @dt.

hint: use lt.db.Exec("SET CLUSTER SETTING ...") in the startup function

Comments from Reviewable

andreimatei · 2017-04-25T22:16:16Z

Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

pkg/settings/registry.go, line 294 at r1 (raw file):

Previously, knz (kena) wrote…

if !ok {
    // Try to see if there is a valid numeric value instead
   var err error
   v, err = strconv.ParseInt(k)
   if err != nil {
       return invalidEnumValueError(e, k)
   }
   isValid := false
   for _, allowedValue := range e.enumValues {
        if v == allowedValue {
           isValid = true
           break
        }
   }
   if !isValid {
       return invalidEnumValueError(e, k)
   }
}
atomic.StoreInt64(&e.v, v)

FWIW, this was a nit meant to tell you about the existence of %q just in case you didn't know about it. I certainly don't feel strongly about such things.

Comments from Reviewable

dt · 2017-04-26T00:06:25Z

Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

pkg/settings/registry.go, line 268 at r1 (raw file):

Previously, arjunravinarayan (Arjun Narayan) wrote…

Done, using IntSetting as the base, since we want to print integers for the String() function.

Not quite: you don't need v or to re-implement Get, when wrapping an existing setting.

Comments from Reviewable

dt · 2017-04-26T00:22:26Z

Review status: all files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

pkg/settings/registry.go, line 279 at r1 (raw file):

Previously, arjunravinarayan (Arjun Narayan) wrote…

I was with @dt, but I'm convinced by the point that allowing names to be changed easily is worth it.

ditto above: it it is just a wrapped IntSetting, don't need to re-implment this.

And I'm still of the opinion that minimizing cognitive load with easily, rapidly understood names is greater value than ability to rename, but if everyone else likes opaque ints, I'll shut up.

pkg/sql/logic_test.go, line 1443 at r2 (raw file):

Previously, knz (kena) wrote…

hint: use lt.db.Exec("SET CLUSTER SETTING ...") in the startup function

Yeah, either do what @knz said, or look at https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/logic_test.go#L1413 (though you might want to use a defer to swap it back if other tests in same pkg care about it).

Comments from Reviewable

rjnn · 2017-04-26T18:37:43Z

RFAL. TFTRs everyone!

Review status: 2 of 9 files reviewed at latest revision, 16 unresolved discussions, some commit checks pending.

pkg/settings/registry.go, line 268 at r1 (raw file):

Previously, dt (David Taylor) wrote…

Not quite: you don't need v or to re-implement Get, when wrapping an existing setting.

Done.

pkg/settings/registry.go, line 294 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

FWIW, this was a nit meant to tell you about the existence of %q just in case you didn't know about it. I certainly don't feel strongly about such things.

Thanks for clarifying :)

pkg/sql/logic_test.go, line 1443 at r2 (raw file):

Previously, dt (David Taylor) wrote…

Yeah, either do what @knz said, or look at https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/logic_test.go#L1413 (though you might want to use a defer to swap it back if other tests in same pkg care about it).

Done. Thank you!

Comments from Reviewable

knz · 2017-04-26T18:49:48Z

Reviewed 3 of 7 files at r5, 4 of 4 files at r6.
Review status: all files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

pkg/sql/logic_test.go, line 1443 at r2 (raw file):

Previously, arjunravinarayan (Arjun Narayan) wrote…

Done. Thank you!

Consider using the new mechanism from #15361 here.

Comments from Reviewable

dt · 2017-04-26T18:59:33Z

Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

pkg/settings/registry.go, line 268 at r1 (raw file):

Previously, arjunravinarayan (Arjun Narayan) wrote…

Done.

Don't need defaultValue either right?

pkg/settings/registry.go, line 294 at r1 (raw file):

Previously, arjunravinarayan (Arjun Narayan) wrote…

Thanks for clarifying :)

this error isn't going to to show up to a user, btw -- set is only called internally.

pkg/settings/registry.go, line 286 at r6 (raw file):

var _ Setting = &EnumSetting{}

func (e *EnumSetting) String() string {

don't need to override this?

pkg/settings/registry.go, line 290 at r6 (raw file):

}

func (e *EnumSetting) setToDefault() {

don't need to override this?

Comments from Reviewable

rjnn · 2017-04-27T18:57:50Z

Review status: 2 of 12 files reviewed at latest revision, 20 unresolved discussions, some commit checks pending.

pkg/settings/registry.go, line 378 at r7 (raw file):

Previously, RaduBerinde wrote…

I see, that is pretty cool. But it should be documented. I would include the description above to one of the functions (e.g. TestingSetBool), and the rest can just say See TestingSetBool for more information.

I would also warn the caller that this cannot be used from tests that could run in parallel with other tests that could use this on the same setting. Not only it could lead to race detector failures, but things can get particularly screwed up if the tests call the cleanup functions out of order.

Thank you Radu. Done.

pkg/settings/registry.go, line 311 at r8 (raw file):

Previously, dt (David Taylor) wrote…

I think you want a v = strings.ToLower(v) before this loop?

strings.ToLower(raw) rather. But yes, thank you.

pkg/settings/registry.go, line 353 at r8 (raw file):

Previously, dt (David Taylor) wrote…

you could probably skip this and let the default value check below panic catch it.

Done.

pkg/sql/logic_test.go, line 1468 at r8 (raw file):

Previously, RaduBerinde wrote…

My suggestion is to add an OverrideDistSQLMode string in ExecutorTestingKnobs and use that if it is set instead of DistSQLClusterExecMode.Get(). We should be able to get to the Executor easily from the two places we are reading it.

Great suggestion, done. Thank you, I did not catch the race possibility.

pkg/sql/set.go, line 182 at r8 (raw file):

Previously, dt (David Taylor) wrote…

I think you want to return the Itoa str of the result of ParseEnum, not the user-provided str, since you want 1, not "auto"` in the the table.

Done.

Comments from Reviewable

RaduBerinde · 2017-04-27T19:08:44Z

Review status: 2 of 12 files reviewed at latest revision, 23 unresolved discussions, some commit checks pending.

pkg/settings/registry.go, line 84 at r10 (raw file):

// TestingSetBool returns a mock, unregistered bool setting for testing. It
// takes a pointer to a BoolSetting reference, swapping in the mock setting.
// It returns a cleanup function that swaps back the original setting. This

Also explain the gossip thing (which explains why we change the pointer). "The original Setting remains registered for gossip-driven updates which become visible when it is restored."

pkg/sql/logic_test.go, line 1473 at r10 (raw file):

					var distSQLOverrideEnum *settings.EnumSetting
					if cfg.overrideDistSQLMode != "" {
						distSQLOverrideEnum = &settings.EnumSetting{}

shouldn't we do *distSQLOverrideEnum = *sql.DistSQLClusterExecMode to initialize enumValues?

Maybe it would be easier to just put the DistSQLExecModeinto the knobs (instead of a setting). Though I also see how a setting could be more powerful in general.

pkg/sql/logic_test.go, line 1474 at r10 (raw file):

					if cfg.overrideDistSQLMode != "" {
						distSQLOverrideEnum = &settings.EnumSetting{}
						settings.TestingSetEnum(&distSQLOverrideEnum, 2)

2? We need to set to overrideDistSQLMode no?

Comments from Reviewable

RaduBerinde · 2017-04-27T19:13:21Z

Review status: 2 of 12 files reviewed at latest revision, 24 unresolved discussions, some commit checks failed.

pkg/sql/vars.go, line 146 at r10 (raw file):

		},
		Reset: func(p *planner) error {
			p.session.DistSQLMode = distSQLExecModeFromInt(DistSQLClusterExecMode.Get())

Need to check override here too. Probably easier if we just store the default mode in p.session (in addition to the current mode) so the override logic is in one place.

Comments from Reviewable

rjnn · 2017-04-27T20:53:41Z

Review status: 2 of 12 files reviewed at latest revision, 24 unresolved discussions, some commit checks pending.

pkg/settings/registry.go, line 84 at r10 (raw file):

Previously, RaduBerinde wrote…

Also explain the gossip thing (which explains why we change the pointer). "The original Setting remains registered for gossip-driven updates which become visible when it is restored."

Done.

pkg/sql/logic_test.go, line 1473 at r10 (raw file):

Previously, RaduBerinde wrote…

shouldn't we do *distSQLOverrideEnum = *sql.DistSQLClusterExecMode to initialize enumValues?

Maybe it would be easier to just put the DistSQLExecModeinto the knobs (instead of a setting). Though I also see how a setting could be more powerful in general.

We don't need to initialize enumValues. In testing, it will just accept a value and eat it quietly.

pkg/sql/logic_test.go, line 1474 at r10 (raw file):

Previously, RaduBerinde wrote…

2? We need to set to overrideDistSQLMode no?

Done.

pkg/sql/vars.go, line 146 at r10 (raw file):

Previously, RaduBerinde wrote…

Need to check override here too. Probably easier if we just store the default mode in p.session (in addition to the current mode) so the override logic is in one place.

I originally didn't because no tests call RESET, but I suppose they might, and when they do, we should do the right thing. Done.

Comments from Reviewable

RaduBerinde · 2017-04-27T21:08:51Z

Thanks for getting this done!

Review status: 2 of 12 files reviewed at latest revision, 23 unresolved discussions, some commit checks failed.

pkg/sql/logic_test.go, line 1473 at r10 (raw file):

Previously, arjunravinarayan (Arjun Narayan) wrote…

We don't need to initialize enumValues. In testing, it will just accept a value and eat it quietly.

Then do we need this line at all? (seems like it would work the same if it's left as nil).

pkg/sql/vars.go, line 146 at r10 (raw file):

Previously, arjunravinarayan (Arjun Narayan) wrote…

I originally didn't because no tests call RESET, but I suppose they might, and when they do, we should do the right thing. Done.

Nice, thanks!

Comments from Reviewable

knz · 2017-04-27T21:37:31Z

LGTM!

Please rebase on top of #15429 to make TC happy. Also look at the linter output.

Reviewed 3 of 9 files at r7, 2 of 7 files at r9, 7 of 7 files at r12.
Review status: all files reviewed at latest revision, 22 unresolved discussions, some commit checks failed.

Comments from Reviewable

bdarnell · 2017-04-27T22:53:56Z

As part of the release plan we want to start testing with distsql=auto on the adriatic/omega/cerulean clusters as soon as possible (without waiting for the other pending distsql PRs). Please deploy a new build to those clusters when this is merged.

Review status: all files reviewed at latest revision, 22 unresolved discussions, some commit checks failed.

Comments from Reviewable

petermattis · 2017-04-28T00:35:27Z

Following up what @bdarnell said, we really want to get this merged tonight and deployed to the test clusters.

petermattis · 2017-04-28T01:06:33Z

Review status: all files reviewed at latest revision, 23 unresolved discussions, some commit checks failed.

pkg/sql/session.go, line 122 at r12 (raw file):

	"sql.defaults.distsql",
	"Default distributed SQL execution mode",
	"Off",

s/Off/Auto/g!!!

Comments from Reviewable

EnumSetting takes a map of acceptable strings to integer values upon creation. EnumSetting accepts strings and if it matches one of the acceptable values, stores the corresponding integer value.

This cluster setting replaces the environment variable. The session variable accessible through `SET distsql = ?` is still available, and defaults to the cluster setting upon session creation. Closes cockroachdb#15045.

rjnn · 2017-04-28T02:30:31Z

TFTRs everyone. Will deploy on release clusters after another PR that enables distsql in auto mode (since that requires changing a few tests that check the distsql mode).

Review status: 4 of 12 files reviewed at latest revision, 20 unresolved discussions, all commit checks successful.

pkg/sql/logic_test.go, line 1473 at r10 (raw file):

Previously, RaduBerinde wrote…

Then do we need this line at all? (seems like it would work the same if it's left as nil).

Yes, we need the line. The TestingSetEnum needs to override the default value, setting it to the thing it reads from cfg.overrideDistSQLMode. Now TestingSetEnum doesn't care about the list of allowable values, so we don't need to initialize enumValues as well. But we still need the default value to be set.

Later, the executor checks if the testing knob is non-nil, and if so, uses the testing mock instead.

pkg/sql/session.go, line 122 at r12 (raw file):

Previously, petermattis (Peter Mattis) wrote…

s/Off/Auto/g!!!

I'll do this in a separate PR. This requires changing a bunch of logic tests that are hardcoded to expect Off. Don't want to comingle any more commits in this PR.

Comments from Reviewable

rjnn requested review from andreimatei and knz April 24, 2017 20:35

rjnn force-pushed the cluster_setting_distsql branch 2 times, most recently from 438d8a9 to 9e159f1 Compare April 24, 2017 20:37

rjnn force-pushed the cluster_setting_distsql branch from 9e159f1 to 7cfc639 Compare April 25, 2017 21:57

rjnn force-pushed the cluster_setting_distsql branch 3 times, most recently from 0e1c71f to d73282a Compare April 26, 2017 18:32

rjnn force-pushed the cluster_setting_distsql branch from d73282a to 6a900ff Compare April 26, 2017 20:49

rjnn force-pushed the cluster_setting_distsql branch from e42522f to 6e96990 Compare April 27, 2017 18:57

rjnn force-pushed the cluster_setting_distsql branch 7 times, most recently from 72b849b to 00f00a4 Compare April 27, 2017 20:51

rjnn force-pushed the cluster_setting_distsql branch from 00f00a4 to 078485b Compare April 27, 2017 20:54

sql: add an enum option for cluster settings

3bda28f

EnumSetting takes a map of acceptable strings to integer values upon creation. EnumSetting accepts strings and if it matches one of the acceptable values, stores the corresponding integer value.

rjnn force-pushed the cluster_setting_distsql branch 3 times, most recently from 110abff to 713e725 Compare April 28, 2017 01:29

distsql: add setting "sql.defaults.distsql"

4534d7c

This cluster setting replaces the environment variable. The session variable accessible through `SET distsql = ?` is still available, and defaults to the cluster setting upon session creation. Closes cockroachdb#15045.

rjnn force-pushed the cluster_setting_distsql branch from 713e725 to 4534d7c Compare April 28, 2017 01:42

rjnn mentioned this pull request Apr 28, 2017

distsql: make 'Auto' the cluster default mode #15442

Merged

rjnn merged commit 021b84c into cockroachdb:master Apr 28, 2017

rjnn deleted the cluster_setting_distsql branch April 28, 2017 02:30

jseldess mentioned this pull request Apr 28, 2017

sql: cluster-wide settings cockroachdb/docs#1277

Closed

rjnn mentioned this pull request May 2, 2017

server: make cluster setting "server.remote_debugging.mode" into an enum setting #15602

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql, distsql: add enum cluster settings, add distsql cluster setting #15307

sql, distsql: add enum cluster settings, add distsql cluster setting #15307

rjnn commented Apr 24, 2017

cockroach-teamcity commented Apr 24, 2017

andreimatei commented Apr 24, 2017

RaduBerinde commented Apr 25, 2017

dt commented Apr 25, 2017

knz commented Apr 25, 2017

knz commented Apr 25, 2017

rjnn commented Apr 25, 2017

knz commented Apr 25, 2017

dt commented Apr 25, 2017

andreimatei commented Apr 25, 2017

andreimatei commented Apr 25, 2017

tamird commented Apr 25, 2017 via email

tamird commented Apr 25, 2017 via email

rjnn commented Apr 25, 2017

knz commented Apr 25, 2017 •

edited

Loading

andreimatei commented Apr 25, 2017

dt commented Apr 26, 2017

dt commented Apr 26, 2017

rjnn commented Apr 26, 2017

knz commented Apr 26, 2017

dt commented Apr 26, 2017

rjnn commented Apr 27, 2017

RaduBerinde commented Apr 27, 2017

RaduBerinde commented Apr 27, 2017

rjnn commented Apr 27, 2017

RaduBerinde commented Apr 27, 2017

knz commented Apr 27, 2017

bdarnell commented Apr 27, 2017

petermattis commented Apr 28, 2017

petermattis commented Apr 28, 2017

rjnn commented Apr 28, 2017

sql, distsql: add enum cluster settings, add distsql cluster setting #15307

sql, distsql: add enum cluster settings, add distsql cluster setting #15307

Conversation

rjnn commented Apr 24, 2017

cockroach-teamcity commented Apr 24, 2017

andreimatei commented Apr 24, 2017

RaduBerinde commented Apr 25, 2017

dt commented Apr 25, 2017

knz commented Apr 25, 2017

knz commented Apr 25, 2017

rjnn commented Apr 25, 2017

knz commented Apr 25, 2017

dt commented Apr 25, 2017

andreimatei commented Apr 25, 2017

andreimatei commented Apr 25, 2017

tamird commented Apr 25, 2017 via email

tamird commented Apr 25, 2017 via email

rjnn commented Apr 25, 2017

knz commented Apr 25, 2017 • edited Loading

andreimatei commented Apr 25, 2017

dt commented Apr 26, 2017

dt commented Apr 26, 2017

rjnn commented Apr 26, 2017

knz commented Apr 26, 2017

dt commented Apr 26, 2017

rjnn commented Apr 27, 2017

RaduBerinde commented Apr 27, 2017

RaduBerinde commented Apr 27, 2017

rjnn commented Apr 27, 2017

RaduBerinde commented Apr 27, 2017

knz commented Apr 27, 2017

bdarnell commented Apr 27, 2017

petermattis commented Apr 28, 2017

petermattis commented Apr 28, 2017

rjnn commented Apr 28, 2017

knz commented Apr 25, 2017 •

edited

Loading