Skip to content

vttestserver: make tablet_refresh_interval configurable and reduce default value#11918

Merged
deepthi merged 11 commits intovitessio:mainfrom
planetscale:vttestserver-fix
Dec 19, 2022
Merged

vttestserver: make tablet_refresh_interval configurable and reduce default value#11918
deepthi merged 11 commits intovitessio:mainfrom
planetscale:vttestserver-fix

Conversation

@rsajwani
Copy link
Contributor

@rsajwani rsajwani commented Dec 8, 2022

Signed-off-by: Rameez Sajwani rameezwazirali@hotmail.com

Description

This is an attempt to make vttestserver faster. Since its used for testing purpose we have seen people finding it slow especially while creating new database. It typically was taking more than 30second (on avg) to create empty database. This PR fixes the issue by reducing the value of.

refreshInterval

Even though we could have reduced the value of other flag

leaderCheckInterval

but we choose not to do that as it is going to be removed in subsequent refactoring PR by @shlomi-noach details here

Reducing these values might put more stress on Topo Server but for testing purpose we should be good. All the remaining details have been captured in the issue #11868

Testing

local testing using a script

bin/vttestserver \
	--port "33804" \
	--keyspaces "unsharded" \
	--num_shards "1" \
	--mysql_bind_host "127.0.0.1" \
	--charset "utf8mb4" \
	--foreign_key_mode "allow" \
	--enable_online_ddl="true" \
	--enable_direct_ddl="true" \
	--planner-version="v3" \
	--vschema_ddl_authorized_users=% \
	--tablet_refresh_interval "11s"
	--schema_dir="/vtdataroot/vt/schema/"

I was able to verify that tablet_refresh_interval is pass all the way to vtcombo

rameezsajwani    95335   0.0  0.2 409314048  53088 s001  S+    1:47PM   0:00.15 /Users/rameezsajwani/Code/fork/vitess/bin/vtcombo --port 33804 --log_dir \ /Users/rameezsajwani/Code/fork/vitess/vtdataroot/vttest1002673685/logs --alsologtostderr --grpc_port 33805 -- \ db_charset utf8mb4 --db_app_user vt_dba --db_app_password  --db_dba_user vt_dba --db_dba_password  \ --proto_topo  keyspaces:{name:"unsharded" shards:{name:"0"} replica_count:2 rdonly_count:1} cells:"test" -- \ mycnf_server_id 1 --mycnf_socket_file \ /Users/rameezsajwani/Code/fork/vitess/vtdataroot/vttest1002673685/vt_0000000001/mysql.sock --normalize_queries \ 
--enable_query_plan_field_caching=false --dbddl_plugin vttest --foreign_key_mode allow --planner-version v3 \ 
--enable_online_ddl=true --enable_direct_ddl=true --enable_system_settings=true \ 
--tablet_refresh_interval=11s --queryserver-config-pool-size 4 --queryserver-config-query-timeout 300 \ 
--queryserver-config-schema-reload-time 60 --queryserver-config-stream-pool-size 4 \ 
--queryserver-config-transaction-cap 4 --queryserver-config-transaction-timeout 300 \
--queryserver-config-txpool-timeout 300 --service_map grpc-vtgateservice,grpc-vtctl,grpc-vtctld \ 
--transaction_mode MULTI --tablet_hostname localhost --vschema_ddl_authorized_users % --db_socket \ /Users/rameezsajwani/Code/fork/vitess/vtdataroot/vttest1002673685/vt_0000000001/mysql.sock -- \ mysql_auth_server_impl none --mysql_server_port 33807 --mysql_server_bind_address 127.0.0.1

setting refreshIntervale to 10 secs, I get following

mysql> CREATE DATABASE `bonjour-vitess5`;
Query OK, 1 row affected (3.03 sec)

mysql> CREATE DATABASE `bonjour-vitess1`;
Query OK, 1 row affected (6.54 sec)

mysql> CREATE DATABASE `bonjour-vitess2`;
Query OK, 1 row affected (6.04 sec)

mysql> CREATE DATABASE `bonjour-vitess3`;
Query OK, 1 row affected (4.53 sec)

Sample log Improvement

I1213 16:57:18.285640   17386 keyspace_events.go:385] created dedicated watcher for keyspace test/bonjour-vitess1
I1213 16:57:21.262830   17386 tablegc.go:271] TableGC: transition into primary
I1213 16:57:26.015104   17386 healthcheck.go:375] Adding tablet to healthcheck: alias:{cell:"test" uid:10} port_map:{key:"grpc" value:9010} port_map:{key:"vt" value:8010} keyspace:"bonjour-vitess1" shard:"0" type:PRIMARY db_name_override:"vt_bonjour-vitess1_0" mysql_port:33806 primary_term_start_time:{seconds:1670979438 nanoseconds:269884000}

Related Issue(s)

closes #11868

Checklist

  • "Backport to:" labels have been added if this change should be back-ported
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@vitess-bot
Copy link
Contributor

vitess-bot bot commented Dec 8, 2022

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.

If a new flag is being introduced:

  • Is it really necessary to add this flag?
  • Flag names should be clear and intuitive (as far as possible)
  • Help text should be descriptive.
  • Flag names should use dashes (-) as word separators rather than underscores (_).

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow should be required, the maintainer team should be notified.

Bug fixes

  • There should be at least one unit or end-to-end test.
  • The Pull Request description should include a link to an issue that describes the bug.

Non-trivial changes

  • There should be some code comments as to why things are implemented the way they are.

New/Existing features

  • Should be documented, either by modifying the existing documentation or creating new documentation.
  • New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • vtctl command output order should be stable and awk-able.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from VTop, if used there.

@rsajwani rsajwani changed the title setting up refresh_interval and check_leader_interval [Do not review] Setting up refresh_interval and check_leader_interval Dec 8, 2022
"--planner-version", args.PlannerVersion,

fmt.Sprintf("--tablet_refresh_interval=%v", args.TopoTabletRefreshInterval),
fmt.Sprintf("--leader_check_interval=%s", "3s"),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be 5 instead of 3?

leaderCheckInterval     = 5 * time.Second

Copy link
Contributor Author

@rsajwani rsajwani Dec 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for vttestserver 5 seems to be high value. In order to make vttestserver fast, we can use 3 instead...

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@rsajwani rsajwani marked this pull request as draft December 12, 2022 15:58
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@rsajwani rsajwani marked this pull request as ready for review December 14, 2022 01:20
@rsajwani rsajwani changed the title [Do not review] Setting up refresh_interval and check_leader_interval Setting up refresh_interval and check_leader_interval Dec 14, 2022
Copy link
Collaborator

@deepthi deepthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work. Mostly naming and config changes in my comments, but it is important to get them right.

Copy link
Contributor

@shlomi-noach shlomi-noach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider my inline comment

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@rsajwani rsajwani added the release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) label Dec 15, 2022
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@deepthi deepthi removed the release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) label Dec 16, 2022
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@deepthi deepthi changed the title Setting up refresh_interval and check_leader_interval vttestserver: make tablet_refresh_interval configurable and reduce default value Dec 19, 2022
@deepthi deepthi merged commit a6ab8a2 into vitessio:main Dec 19, 2022
@deepthi deepthi deleted the vttestserver-fix branch December 19, 2022 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Question: It takes minimum 40s to create a new empty database using vitess/vttestserver:mysql80 - Could this be improved?

4 participants