Generate config.yaml when running sql-server #8644

milogreg · 2024-12-05T23:12:36Z

If sql-server is ran without a specified config file and there is no config.yaml in the database directory, one will be generated.

The rules for how the default config.yaml file is generated are as follows:

If a field has a value defined by the sql-server execution (such as through CLI args), then that value will be used.
If a field has no set value but has a default value, then that default will be used.
If a field has no set value and no default, a commented-out line setting the field to null will be included as a placeholder.

Part of #7980

…ready exist

…them null

timsehn · 2024-12-05T23:20:56Z

Very cool!

My first comment is that it could use some bats tests. It probably should have its own test file.

Here's an example to crib from:

https://github.com/dolthub/dolt/blob/main/integration-tests/bats/sql-server.bats

Also, the bats readme can help you get started with running your tests:

https://github.com/dolthub/dolt/tree/main/integration-tests/bats#readme

milogreg · 2024-12-09T00:54:26Z

Made some bats tests.

However, I noticed some other tests using sql-server skip when running windows due to issues I'm unaware of, so I'm not sure if the tests will behave properly on windows yet (I've only ran them on Mac).

timsehn · 2024-12-09T17:04:24Z

@jycor will review this today.

zachmu · 2024-12-09T22:42:06Z

Some of these bats tests are failing because we just removed a couple of config keys the tests are expecting. Merge main, run locally, and fix the tests.

zachmu · 2024-12-09T23:04:31Z

Also @milogreg you should join the discord to talk in real time for faster turnaround on these issues.

jycor

Thanks for this contribution! I like the high level idea of this PR.
The BATS are very thorough and well tested!

However, I don't like the use of reflect for fillDefault. I think it would be better to explicitly check each field of the struct and fill accordingly.

Lastly, I'm concerned about how much longer BATS is taking. I'm not sure if it's a side effect of github actions on a PR from a fork or if the config generation is slowing it down signigicantly.

zachmu

Functionality looks good but see comments

go/cmd/dolt/commands/sqlserver/sqlserver.go

go/libraries/doltcore/servercfg/yaml_config.go

go/libraries/doltcore/servercfg/yaml_config_test.go

go/libraries/doltcore/servercfg/yaml_config.go

integration-tests/bats/sql-server-config-file-generation.bats

milogreg · 2024-12-10T00:35:58Z

Thanks for this contribution! I like the high level idea of this PR. The BATS are very thorough and well tested!

However, I don't like the use of reflect for fillDefault. I think it would be better to explicitly check each field of the struct and fill accordingly.

Lastly, I'm concerned about how much longer BATS is taking. I'm not sure if it's a side effect of github actions on a PR from a fork or if the config generation is slowing it down signigicantly.

I'll remove that usage of reflect, and look into potential performance issues.

I didn't put much thought into performance when designing this, as I falsely assumed that sql-server wasn't something that would be ran repeatedly from a clean state (I clearly didn't take testing into account). So, I wouldn't be surprised at all if the config generation is the cause of the slowdown.

The first thing I'll try is removing the expensive usages of reflection (aside from the ones in the YAML marshaling itself), and see how that affects the runtime (hopefully this is all it will take to make it performant). If that isn't enough, I'll try to actually nail down what the bottleneck is and redesign accordingly.

However, I don't think I know enough to determine what a reasonable target time per config generation would be, is there an approximate maximum time that would be acceptable? I'll be benchmarking it on my m3 MacBook pro.

milogreg · 2024-12-10T01:11:54Z

From my extremely quick testing it looks like almost all of the overhead of the config generation comes from my usage of dEnv.FS.WriteFile, which is many times slower than os.WriteFile. I think I just did it like this because I saw other parts of the codebase write files in this way. Is it ok to switch to os.WriteFile, or is there another more proper way of writing files faster?

…abase directory

milogreg · 2024-12-10T21:00:11Z

I looked into the dEnv.FS.WriteFile performance difference, it looks like it's significantly slower than os.WriteFile because it writes the file atomically. I think the implementation of dEnv.FS.WriteFile (WriteFileAtomically in go/libraries/utils/file/sync.go) is pretty optimal, so this overhead may be unavoidable unless we want to give up the atomic guarantee. I can't think of a major reason as to why it would be bad to leave an incomplete config.yaml in the event of a crash, so I'd assume it would be okay to use the non-atomic WriteFile, but I wont make that decision by myself. Another option, for if an atomic write isn't needed but using dEnv.FS is required, is to use dEnv.FS.OpenForWrite (which is about as performant as os.WriteFile).

zachmu

Thanks for the tests, seeing a concrete example of the output made it clear how this could be improved.

This is getting close, appreciate the effort on this.

go/libraries/doltcore/servercfg/yaml_config.go

integration-tests/bats/sql-server-config-file-generation.bats

zachmu · 2024-12-12T20:18:25Z

go/libraries/doltcore/servercfg/yaml_config_test.go

@@ -415,3 +414,33 @@ listener:
 	err = ValidateConfig(cfg)
 	assert.Error(t, err)
 }
+
+func TestCommentNullYAMLValues(t *testing.T) {


This test is good but this is important enough behavior we want to exercise it a little more thoroughly, and to be more end-to-end. These are nice because it lets a human reviewer see what the behavior is explicitly.

When I see it spelled out like this, I notice the behavior here isn't quite what we want. A commented-out line with value1: null tells the user there's a key called value1 but doesn't tell them what a normal value should be if they decide they want to set it. The goal is to create a self-documenting process where a customer gets a file created that they can use to learn about the configuration options and change them themselves going forward.

Better end-to-end behavior would be:

Every key explicitly specified via command line flags is set

Every other key is commented out with its default value (including those in structs)

Config elements that currently have no default defined need example values to server as placeholders for this use case, rather than null. You will need to define these explicitly using the docs as a guide.

A comment at the top of the file indicating it was generated and that users should uncomment and edit lines as necessary.

With all that in place, what I want to see is one or more Go tests like this one, but for the entire process end-to-end, i.e. "given these command line options, the following config file is produced verbatim."

zachmu · 2024-12-18T21:24:34Z

Looks like you'll need to merge main again.

zachmu · 2025-01-02T17:52:35Z

@milogreg do you still want to work on this? We can probably run it home for you if not. Let us know either way.

milogreg · 2025-01-04T00:22:14Z

Thanks for checking in, I plan to work on it over the next week.

…nd to always include a commented-out placeholder for unset fields, even if no default value exists

…-level fields themselves (fixing awkward newlines in top-level fields containing lists

…oduce equivalent YAML

…other fields

… for a given set of command line args

zachmu

This looks really good, just minor comments.

There's one field which is internal only / undocumented, so we don't want it showing up in the generated config file. Up to you how to best address that.

zachmu · 2025-01-07T23:11:39Z

go/cmd/dolt/commands/sqlserver/sqlserver.go

+	const yamlConfigName = "config.yaml"
+
+	apr := cli.ParseArgsOrDie(ap, args, help)
+	if err := validateSqlServerArgs(apr); err != nil {


This is redundant, already happens in ServerConfigFromArgs

zachmu · 2025-01-07T23:37:14Z

go/cmd/dolt/commands/sqlserver/sqlserver.go

+	generatedYaml := `# This file was generated using your configuration.
+# Uncomment and edit lines as necessary to modify your configuration.` + "\n\n" + yamlConfig.VerboseString()


Suggested change

generatedYaml := `# This file was generated using your configuration.

# Uncomment and edit lines as necessary to modify your configuration.` + "\n\n" + yamlConfig.VerboseString()

generatedYaml := `# Dolt SQL server configuration

#

# Uncomment and edit lines as necessary to modify your configuration.

# Full documentation: https://docs.dolthub.com/sql-reference/server/configuration

#` + "\n\n" + yamlConfig.VerboseString()

zachmu · 2025-01-07T23:45:16Z

go/libraries/doltcore/servercfg/yaml_config.go

+	}
+
+	if validatingCfg, ok := cfg.(ValidatingServerConfig); ok {
+		res.GoldenMysqlConn = zeroIf(ptr(validatingCfg.GoldenMysqlConnectionString()), !cfg.ValueSet(GoldenMysqlConnectionStringKey))


This setting is internal only and undocumented, let's omit.

zachmu · 2025-01-07T23:51:12Z

go/libraries/doltcore/servercfg/yaml_config.go

+		}
+	}
+	if withPlaceholders.MetricsConfig.Host == nil {
+		withPlaceholders.MetricsConfig.Host = ptr("123.45.67.89")


Suggested change

withPlaceholders.MetricsConfig.Host = ptr("123.45.67.89")

withPlaceholders.MetricsConfig.Host = ptr("localhost")

zachmu · 2025-01-07T23:57:22Z

go/libraries/doltcore/servercfg/yaml_config.go

+
+	if withPlaceholders.MetricsConfig.Labels == nil {
+		withPlaceholders.MetricsConfig.Labels = map[string]string{
+			"label1": "value1",


Default for this should be an empty map (docs are confusing, it's a very advanced use case)

zachmu · 2025-01-07T23:58:50Z

go/libraries/doltcore/servercfg/yaml_config.go

+		}
+	}
+
+	if withPlaceholders.GoldenMysqlConn == nil {


Definitely omit this from the final output

zachmu · 2025-01-08T00:00:05Z

go/libraries/doltcore/servercfg/yaml_config.go

+		}
+	}
+
+	if withPlaceholders.Jwks == nil {


Default for this field should be empty (it's advanced and poorly documented, currently only used for our hosting service)

zachmu · 2025-01-08T00:04:26Z

go/libraries/doltcore/servercfg/yaml_config.go

-	MetricsConfig     MetricsYAMLConfig      `yaml:"metrics"`
-	RemotesapiConfig  RemotesapiYAMLConfig   `yaml:"remotesapi"`
+	MetricsConfig     MetricsYAMLConfig      `yaml:"metrics,omitempty"`
+	RemotesapiConfig  RemotesapiYAMLConfig   `yaml:"remotesapi,omitempty"`


We want ClusterConfig and MetricsConfig and Jwks the bottom of the file, since they are only for advanced users. Since the yaml marshaler outputs in struct field order, that means we should move these fields to the end of the struct.

milogreg added 3 commits December 4, 2024 17:41

Generate config.yaml with current sql server config if one doesn't al…

81e21a6

…ready exist

Fill default values in YAML config if they exist rather than leaving …

0df8e9a

…them null

Comment out null values in generated YAML config

3b24a0c

coffeegoddd added the contribution label Dec 5, 2024

milogreg force-pushed the config-file-gen branch from fb7a884 to e196001 Compare December 9, 2024 00:32

Add bats tests for config file generation

1b407e0

milogreg force-pushed the config-file-gen branch from e196001 to 1b407e0 Compare December 9, 2024 00:41

Skip bats tests when SQL_ENGINE is set to remote-engine

5f4f2d1

milogreg added 2 commits December 9, 2024 15:13

Merge main

698cc63

Don't use removed persistence_behavior config option in tests

27dcc30

jycor reviewed Dec 9, 2024

View reviewed changes

zachmu reviewed Dec 10, 2024

View reviewed changes

milogreg added 5 commits December 9, 2024 17:40

Make default filler not use reflection

6f89115

Remove circular reference test

2654f84

Add test for commentNullYAMLValues

211a52e

Remove CLI warning for when directory named config.yaml exists in dat…

bc409a6

…abase directory

Add additional doc comments for functions

b316473

Merge main

bd4c410

milogreg requested review from zachmu and jycor December 11, 2024 18:20

zachmu reviewed Dec 12, 2024

View reviewed changes

milogreg and others added 11 commits January 6, 2025 11:22

Merge main

7627f6b

Rework config file generation to only include fields explicitly set a…

290d0d2

…nd to always include a commented-out placeholder for unset fields, even if no default value exists

Make YAML formatting not treat list items in a top-level field as top…

96e2200

…-level fields themselves (fixing awkward newlines in top-level fields containing lists

Update minver_validation.txt

6eb9959

Update config file gen bats test

86446e6

Add test to ensure YAMLConfig's String and VerboseString functions pr…

7a3b62e

…oduce equivalent YAML

Add test for commentYAMLDiffs

a3e3b11

Merge branch 'main' into config-file-gen

c70c87f

Fix docs for generateYamlConfigIfNone

3a2e999

Make logic for checking mysql connection string more consistent with …

c72b061

…other fields

Add test to ensure that an expected YAML config is generated verbatim…

70d5372

… for a given set of command line args

milogreg requested a review from zachmu January 8, 2025 00:04

zachmu reviewed Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate config.yaml when running sql-server #8644

Generate config.yaml when running sql-server #8644

milogreg commented Dec 5, 2024

timsehn commented Dec 5, 2024

milogreg commented Dec 9, 2024

timsehn commented Dec 9, 2024

zachmu commented Dec 9, 2024

zachmu commented Dec 9, 2024

jycor left a comment •

edited

Loading

zachmu left a comment

milogreg commented Dec 10, 2024

milogreg commented Dec 10, 2024

milogreg commented Dec 10, 2024

zachmu left a comment

zachmu Dec 12, 2024

zachmu commented Dec 18, 2024

zachmu commented Jan 2, 2025

milogreg commented Jan 4, 2025

zachmu left a comment

zachmu Jan 7, 2025

zachmu Jan 7, 2025

zachmu Jan 7, 2025

zachmu Jan 7, 2025

zachmu Jan 7, 2025

zachmu Jan 7, 2025

zachmu Jan 8, 2025

zachmu Jan 8, 2025

		generatedYaml := `# This file was generated using your configuration.
		# Uncomment and edit lines as necessary to modify your configuration.` + "\n\n" + yamlConfig.VerboseString()

	withPlaceholders.MetricsConfig.Host = ptr("123.45.67.89")
	withPlaceholders.MetricsConfig.Host = ptr("localhost")

Generate config.yaml when running sql-server #8644

Are you sure you want to change the base?

Generate config.yaml when running sql-server #8644

Conversation

milogreg commented Dec 5, 2024

timsehn commented Dec 5, 2024

milogreg commented Dec 9, 2024

timsehn commented Dec 9, 2024

zachmu commented Dec 9, 2024

zachmu commented Dec 9, 2024

jycor left a comment • edited Loading

Choose a reason for hiding this comment

zachmu left a comment

Choose a reason for hiding this comment

milogreg commented Dec 10, 2024

milogreg commented Dec 10, 2024

milogreg commented Dec 10, 2024

zachmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachmu commented Dec 18, 2024

zachmu commented Jan 2, 2025

milogreg commented Jan 4, 2025

zachmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jycor left a comment •

edited

Loading