Feature/add ti resources #21

boltandrke · 2022-08-09T14:45:02Z

This commit will add dynamic TiKV and PD resource handling with TiDB cluster and closes issue #15
All supported TiKV and PD configuration parameters are described here

Merge upstream master

borissavelev · 2022-08-12T09:30:27Z

@petoju sorry for pinging, could you please take a look?)

petoju

@borissavelev sorry, I was on vacation for this week and I plan being on one in the next week.

Except for some details, it looks reasonably well. I can still merge it if I get reasonable replies or changes.

petoju · 2022-08-12T20:06:27Z

GNUmakefile

@@ -52,12 +54,21 @@ testpercona:
 testtidb%:
 	$(MAKE) MYSQL_VERSION=$* MYSQL_PORT=34$(shell echo "$*" | tr -d '.') testtidb

-testtidb:
-	-docker run --rm --name test-tidb$(MYSQL_VERSION) -d -p $(MYSQL_PORT):4000 pingcap/tidb:v$(MYSQL_VERSION)
+bin/tiup:


Could we do the same in Docker?

Your approach forces you to start considering arch, runs with nohup, depends on some mirror without chance to cache it, there may be potentially be issues with binary compatibility on newer GLIBC and so on.

Yes, its possible with version 4.0 as described here. But its not straight forward setup as tiup does.
Starting from version 5.0 docker is not anymore supported method pulling up the cluster.
This tool is needed only for acceptance testing. I'll glue it to am64 arch only. (similar like terraform binary fetch).

Pingcap still publishes the docker-compose config: https://github.com/pingcap/tidb-docker-compose - but I admit it may be more complicated.

Still, if we don't want to use that - why should it run under nohup? Should it be left running in the background?

Replaced tiup with minimal docker setup. PTAL

Thanks, this looks good. There is a minor issue there and I have / already had few more questions (replying you considered those is good enough for me). After that is done, I can merge it.

petoju · 2022-08-12T20:15:47Z

mysql/resource_ti_config_variable.go

+	if isNumeric(varValue) {
+		configQuery = fmt.Sprintf("%s %s", configQuery, varValue)
+	} else {
+		configQuery = fmt.Sprintf("%s'%s'", configQuery, varValue)


What if varValue contains apostrophes? Even spitting out a warning is better than nothing.

Here is an example If subsystem doesn't accept apostrophes

mysql> set config pd log.level="warn`";
Query OK, 0 rows affected, 1 warning (0.01 sec)

mysql> show warnings;
+---------+------+---------------------------------------------------------------------------------------+
| Level | Code | Message |
+---------+------+---------------------------------------------------------------------------------------+
| Warning | 1105 | bad request to http://127.0.0.1:2379/pd/api/v1/config: "log level warn is illegal"
|
+---------+------+---------------------------------------------------------------------------------------+
1 row in set (0.00 sec)
`

It will be later rejected by warning check.

I'll add extra logging about this

Ok, my question was maybe laid incorrectly.

Can it happen, that some value will contain apostrophe = ' character? In that case, your SQL will be invalid. BTW your new check for backticks ` seems to be broken (like it will warn almost every single time).

For example, is it possible or likely, that a human will want to set some value to something'somethingelse? If that is possible, then we should certainly add some escaping.

Thank you for pointing out this. I added validator for this.

petoju · 2022-08-12T20:20:10Z

mysql/resource_ti_config_variable_test.go

+func testAccConfigVarConfig_withInstanceAndType(varName string, varValue string, varType string, varInstance string) string {
+	return fmt.Sprintf(`
+resource "mysql_ti_config" "test" {
+  name = "%s"


Indent it please.

petoju · 2022-08-12T20:22:46Z

mysql/resource_ti_config_variable.go

+	d.Set("value", defaultValue.String())
+
+	return CreateOrUpdateConfigVariable(d, meta)
+


No newline after return, please.

mysql/resource_ti_config_defaults.go

mysql/resource_ti_config_variable.go

mysql/resource_ti_config_variable_test.go

petoju · 2022-08-12T20:39:20Z

mysql/resource_ti_config_variable.go

+		configQuery = fmt.Sprintf("SET CONFIG \"%s\" %s=", varInstance, quoteIdentifier(varName))
+	}
+
+	if isNumeric(varValue) {


This doesn't sound right. For DBs respecting data types, it depends on the type of key. Type of value may help (maybe that's your approximation), but it doesn't always work.

Doesn't TiDB respect it (like MySQL) in case you quote everything?

L64:
If instance is in place then it requires extra quoting like in the example
set config "127.0.0.1:20180" split.qps-threshold=1000

Everything else should be configured like this
set config tikv split.qps-threshold=1000

ref. https://docs.pingcap.com/tidb/stable/dynamic-config

L64: If instance is in place then it requires extra quoting like in the example set config "127.0.0.1:20180" `split.qps-threshold`=1000

Everything else should be configured like this set config tikv `split.qps-threshold`=1000

We don't understand each other - is it bad, if you quote it like this?

set config tikv `split.qps-threshold`='1000'

I believe it would be also acceptable and it would make it a bit easier to understand, but I'd leave it up to you.

Got it 😀 Thank you 👍
Removed obsolete code

scripts/tidb-test-cluster.sh

petoju · 2022-08-17T20:52:12Z

GNUmakefile

@@ -52,12 +54,21 @@ testpercona:
 testtidb%:
 	$(MAKE) MYSQL_VERSION=$* MYSQL_PORT=34$(shell echo "$*" | tr -d '.') testtidb

-testtidb:
-	-docker run --rm --name test-tidb$(MYSQL_VERSION) -d -p $(MYSQL_PORT):4000 pingcap/tidb:v$(MYSQL_VERSION)
+bin/tiup:


Thanks, this looks good. There is a minor issue there and I have / already had few more questions (replying you considered those is good enough for me). After that is done, I can merge it.

boltandrke · 2022-08-18T09:41:01Z

If I understood correctly, everything should be fixed now.
Before you merge it, let me cleanup branch with rebase. :)

petoju

Looks good now - thanks.

I'm not merging it yet as you requested a rebase.

boltandrke · 2022-08-19T07:53:27Z

Thank you!🙏 Now its clean. All acceptance tests passed and we are 🟢 to merge it.
Its was pleasure to work with you 🙇‍♂️

Merge pull request #1 from petoju/master

4d45c99

Merge upstream master

boltandrke force-pushed the feature/add-ti-resources branch from 3a19635 to ed35e60 Compare August 10, 2022 06:58

petoju requested changes Aug 12, 2022

View reviewed changes

boltandrke added a commit to boltandrke/terraform-provider-mysql that referenced this pull request Aug 15, 2022

Fix PR petoju#21 comments

480d8b1

boltandrke requested a review from petoju August 15, 2022 13:21

petoju reviewed Aug 17, 2022

View reviewed changes

petoju approved these changes Aug 19, 2022

View reviewed changes

Add TiKV and PD resource handling with TiDB cluster

4297527

boltandrke force-pushed the feature/add-ti-resources branch from 80767d3 to 4297527 Compare August 19, 2022 06:38

boltandrke requested a review from petoju August 19, 2022 10:06

petoju merged commit a7b8c1f into petoju:master Aug 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/add ti resources #21

Feature/add ti resources #21

boltandrke commented Aug 9, 2022 •

edited

Loading

borissavelev commented Aug 12, 2022

petoju left a comment •

edited

Loading

petoju Aug 12, 2022

boltandrke Aug 15, 2022

petoju Aug 15, 2022

boltandrke Aug 17, 2022 •

edited

Loading

petoju Aug 17, 2022

petoju Aug 12, 2022

boltandrke Aug 15, 2022

petoju Aug 15, 2022

boltandrke Aug 18, 2022

petoju Aug 12, 2022

petoju Aug 12, 2022

boltandrke Aug 15, 2022

petoju Aug 12, 2022

boltandrke Aug 15, 2022

petoju Aug 15, 2022

boltandrke Aug 18, 2022 •

edited

Loading

petoju Aug 17, 2022

boltandrke commented Aug 18, 2022

petoju left a comment

boltandrke commented Aug 19, 2022

		d.Set("value", defaultValue.String())

		return CreateOrUpdateConfigVariable(d, meta)

Feature/add ti resources #21

Feature/add ti resources #21

Conversation

boltandrke commented Aug 9, 2022 • edited Loading

borissavelev commented Aug 12, 2022

petoju left a comment • edited Loading

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

boltandrke Aug 17, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boltandrke Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boltandrke commented Aug 18, 2022

petoju left a comment

Choose a reason for hiding this comment

boltandrke commented Aug 19, 2022

boltandrke commented Aug 9, 2022 •

edited

Loading

petoju left a comment •

edited

Loading

boltandrke Aug 17, 2022 •

edited

Loading

boltandrke Aug 18, 2022 •

edited

Loading