settings: slotIdx fix

[jiadexin]

Fixes #38786.

i modified 2 methods : settingChanged and setOnChange, use slotIdx-1 to access elements.

[cockroach-teamcity]

This change is 

[CLAassistant]

All committers have signed the CLA.

[craig]

🔒 Permission denied

Existing reviewers: click here to make jiadexin a reviewer

[ajwerner]

Thank you for making this change!

This change could benefit greatly from a simple unit that registers and on change function and then verifies that it is called when that setting is changed.

I agree with @andreimatei's original assessment that we should probably zero-index these settings but that doesn't need to happen here. Please include add a test and we can get this merged.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @andreimatei, and @tbg)

[jiadexin]

i already add a test of "maxSettings"

[ajwerner]

Thanks! Now there's just some clean up left before we can get this in.

We adhere relatively strictly to the style guide. In particular we prefer comments to be formed as sentences with a space between // and the content, a capitalized first letter and a terminating .. See https://github.com/golang/go/wiki/CodeReviewComments#comment-sentences

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @jiadexin, and @tbg)

---

pkg/settings/settings_test.go, line 619 at r2 (raw file):

	maxSettings := 128
	var err error
	//register max settings

For example, this should be:

// Register maxSettings settings to ensure that no errors occur.

---

pkg/settings/settings_test.go, line 637 at r2 (raw file):

	u := settings.NewUpdater(sv)
	u.Set(maxName, settings.EncodeInt(9), "i")

The linter is complaining that this error isn't checked. Can you assert that the error returned from Set is nil?

---

pkg/settings/settings_test.go, line 643 at r2 (raw file):

	}

	//register too many setting

This part feels like it should be a separate test.

---

pkg/settings/settings_test.go, line 650 at r2 (raw file):

	}
}
func batchRegisterSettings(t *testing.T, keyPrefix string, count int, err *error) string {

It's more idiomatic to return (maxSettingName string, err error) than to the the pointer as a return value.

For example:

func batchRegisterSettings(t *testing.T, keyPrefix string, count int, err *error) (maxName string, err error) {
	defer func() {
		//catch painc error, and convert to error
		if r := recover(); r != nil {
			// Check exactly what the panic was and convert it into a return error.
			switch x := r.(type) {
			case error:
				err = x
			default:
				err = errors.Errorf("panic: %v", x)
			}
		}
        }()
        ...
}

[jiadexin]

@ajwerner thanks for your review.
1.I already modify comments.
2. //register too many setting : it should be another test. I cannot find any test about const maxSettings = 128, so i add it. Now, i removed it from this commit.
3.batchRegisterSettings method: the panic error can not as a return value,so it should be convert to error in defer func(), and only use pointer. I don't know any other methods to do it.

[ajwerner]

Thanks for keeping at it! This needs another pass on the cleanup.

2. //register too many setting : it should be another test. I cannot find any test about const maxSettings = 128, so i add it. Now, i removed it from this commit.

It's a good test, just felt like you could move it out into its own top-level test.

3.batchRegisterSettings method: the panic error can not as a return value,so it should be convert to error in defer func(), and only use pointer. I don't know any other methods to do it.

See my comment below. This golang snippet demonstrates the technique https://play.golang.org/p/Gz85gz5gW4c

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @jiadexin, and @tbg)

---

pkg/settings/settings_test.go, line 643 at r2 (raw file):

Previously, ajwerner wrote…

This part feels like it should be a separate test.

I liked that you were testing for the overflow panic, I was merely suggesting that you should write another top-level test for that logic, for exampleTestMaxSettingsPanics.

Furthermore, if you knew that only this call which overflows the number of setting panics, you could recover from the panic inside of the test function rather than inside of batchRegisterSettings.

---

pkg/settings/settings_test.go, line 650 at r2 (raw file):

3.batchRegisterSettings method: the panic error can not as a return value,so it should be convert to error in defer func(), and only use pointer. I don't know any other methods to do it.

You can refer to named return values in a deferred function. See this example. Here err is the return value.

---

pkg/settings/settings_test.go, line 628 at r3 (raw file):

	sv := &settings.Values{}
	sv.Init(settings.TestOpaque)
	{

Why the { block?

---

pkg/settings/settings_test.go, line 645 at r3 (raw file):

		}
	}
}

nit: add a new line.

---

pkg/settings/settings_test.go, line 648 at r3 (raw file):

func batchRegisterSettings(t *testing.T, keyPrefix string, count int, err *error) string {
	defer func() {
		// Catch painc error, and convert to error

nit: .

[jiadexin]

@ajwerner Thank you, and i learned new knowledge again.

[ajwerner]

Very close. One more pass to clean things up and get the tests passing and we can get this in.

Reviewed 1 of 1 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @jiadexin, and @tbg)

---

pkg/settings/settings_test.go, line 645 at r3 (raw file):

Previously, ajwerner wrote…

nit: add a new line.

Please add a new line before batchRegisterSettings.

---

pkg/settings/settings_test.go, line 617 at r4 (raw file):

func TestMaxSlotIdxSettings(t *testing.T) {
	maxSettings := 128

nit: define outside of this function to share between the two tests

const maxSettings = 128

---

pkg/settings/settings_test.go, line 630 at r4 (raw file):

	intSetting, ok := settings.Lookup(maxName)
	if !ok {
		t.Errorf("expected lookup setting : %s", maxName)

The colon is in a weird place. How about:

t.Errorf("expected lookup of %s to succeed", maxName)

---

pkg/settings/settings_test.go, line 640 at r4 (raw file):

	if changes != 1 {
		t.Errorf("expected the max slot setting changed .")

nit: remove the space between changed and .

---

pkg/settings/settings_test.go, line 646 at r4 (raw file):

func TestMaxSettingsPanics(t *testing.T) {
	maxSettings := 129
	// Register too many settings, this will cause errors.

Let's be a little bit more specific:

// Register too many settings which will cause a panic which is caught and converted to an error.

---

pkg/settings/settings_test.go, line 647 at r4 (raw file):

	maxSettings := 129
	// Register too many settings, this will cause errors.
	_, err := batchRegisterSettings(t, "i.Val", maxSettings-1-len(settings.Keys()))

This test is failing because you've already registered a key of this name from the previous test. Consider passing t.Name() as the key prefix here and above to fix the test.

---

pkg/settings/settings_test.go, line 650 at r4 (raw file):

	expectedErr := "too many settings; increase maxSettings"
	if err == nil || err.Error() != expectedErr {
		t.Errorf("expected error :'%s' error, but get : %s", expectedErr, err)

The : is in a weird place here. I'm not sure it's the right punctuation anyway. How about:

t.Errorf("expected error %v, but got %v",  expectedErr, err)

---

pkg/settings/settings_test.go, line 655 at r4 (raw file):

func batchRegisterSettings(t *testing.T, keyPrefix string, count int) (name string, err error) {
	defer func() {
		// Catch painc error, and convert to error.

spelling of panic.

I might write this as:

Catch panic and convert it to an error.

---

pkg/settings/settings_test.go, line 664 at r4 (raw file):

				err = x
			default:
				err = errors.New("Unknow panic")

Spelling of Unknown. May as well capture also the panic message with

errors.Errorf("unknown panic: %v", x)

Also style dictates that error messages not begin with capital letters.

[ajwerner]

Great! Now this is looking pretty good. The last thing is to squash these commits into a single commute. Otherwise we’d have WIP commits in the master branch that wouldn't pass tests and would make bisect harder to use. Once you've squashed the commits, force-pushed, and get this to build then we'll merge it. Thanks again for keeping at it.

Reviewed 1 of 1 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @jiadexin, and @tbg)

---

pkg/settings/settings_test.go, line 618 at r5 (raw file):

}

func TestMaxSlotIdxSettings(t *testing.T) {

final nit: call this TestOnChangeWithMaxSettings or something like that.

---

pkg/settings/settings_test.go, line 650 at r5 (raw file):

	expectedErr := "too many settings; increase maxSettings"
	if err == nil || err.Error() != expectedErr {
		t.Errorf("expected error %v, but got %v",  expectedErr, err)

There's an extra space here that our gofmt linter is complaining about.

[ajwerner]

Okay very very close. Please update the commit message for the one commit. It should have a prefix of settings: and a clear message. Perhaps settings: fix panic in SetOnChange for max setting

Reviewed 1 of 1 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @jiadexin, and @tbg)

[ajwerner]

LGTM. I appreciate your sticking with it.

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)