Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/dolt/commands/sqlserver: Restructure the start up sequence for sql-server. #7004

Merged
merged 8 commits into from
Nov 16, 2023

Conversation

reltuk
Copy link
Contributor

@reltuk reltuk commented Nov 15, 2023

We explicitly model Services, which can have an Init step, a Run step and a Stop step. Every registered service get initialized in the order they were registered in, then they all run concurrently until Stop is called, when they all get Stopped in reverse order. It's possible for clients to wait for init to be completed and be delivered any errors encountered on startup. They can also wait for stop, to be delivered any errors encountered on shutdown.

…l-server.

We explicitly model Services, which can have an Init step, a Run step and a
Stop step. Every registered service get initialized in the order they were
registered in, then they all run concurrently until Stop is called, when they
all get Stopped in reverse order. It's possible for clients to wait for init to
be completed and be delivered any errors encountered on startup. They can also
wait for stop, to be delivered any errors encountered on shutdown.
go/cmd/dolt/commands/sqlserver/server.go Outdated Show resolved Hide resolved
go/cmd/dolt/commands/sqlserver/server.go Outdated Show resolved Hide resolved
go/cmd/dolt/commands/sqlserver/server.go Outdated Show resolved Hide resolved
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

So overall I think there's a good abstraction here, but I'm not wild about the current implementation details. Basically boils down to 3 big points:

  1. There's leakage from one service's Init to another, e.g. clusterController gets Init'ed by one service then used by the Init of another one. These things should be reasonably self-contained
  2. There are many Init-only "Services", which is another sign of a leaky abstraction. A thing that just runs an Init block and doesn't even need the cleanup of Close() isn't a service and shouldn't be modeled as one
  3. The actual services should be better encapsulated with their own types and probably their own files, not just anonymous functions. Service should probably be an interface, not a struct with funcs.

Basically I think you need to do a better job modeling and separating the concerns of initialization from the concerns of running a service and cleaning up when it shuts down.

…rSpecified variable in the InitSuperUser step.
…ce instead of a struct. Experiment with ServiceState and more rigorous cleanup of some services.
…tesapi servers if we never get to the run step.
… mySQLServer instance, even if we never get to the service which is responsible for running it.
…concurrently and multiple times and it never returns an error.
@reltuk reltuk marked this pull request as ready for review November 15, 2023 21:35
@reltuk
Copy link
Contributor Author

reltuk commented Nov 15, 2023

I moved Service to an interface but I ended up leaving all the existing uses as instances of a struct implementation. When I teased things apart a bit further, if felt like I was getting less readable code, à la http://number-none.com/blow/john_carmack_on_inlined_code.html ...which, to be fair, we do have some gross mutations in here.

@reltuk reltuk merged commit 820686b into main Nov 16, 2023
17 checks passed
Copy link

@coffeegoddd DOLT

test_name detail row_cnt sorted mysql_time sql_mult cli_mult
batching LOAD DATA 10000 1 0.05 0.6
batching batch sql 10000 1 0.08 1.25
batching by line sql 10000 1 0.08 1.25
blob 1 blob 200000 1 0.89 3.09 3.54
blob 2 blobs 200000 1 0.85 4.14 4.74
blob no blob 200000 1 0.86 1.35 1.44
col type datetime 200000 1 0.81 1.8 1.93
col type varchar 200000 1 0.68 1.82 1.9
config width 2 cols 200000 1 0.79 1.41 1.28
config width 32 cols 200000 1 1.84 1.53 2.49
config width 8 cols 200000 1 0.93 1.44 1.66
pk type float 200000 1 0.84 1.26 1.26
pk type int 200000 1 0.78 1.27 1.31
pk type varchar 200000 1 1.51 1.01 0.99
row count 1.6mm 1600000 1 5.6 1.5 1.53
row count 400k 400000 1 1.38 1.46 1.49
row count 800k 800000 1 2.84 1.47 1.48
secondary index four index 200000 1 3.59 1.08 0.91
secondary index no secondary 200000 1 0.86 1.35 1.42
secondary index one index 200000 1 1.12 1.49 1.45
secondary index two index 200000 1 1.99 1.2 1.09
sorting shuffled 1mm 1000000 0 5.03 1.84 1.82
sorting sorted 1mm 1000000 1 5.07 1.84 1.81

Copy link

@coffeegoddd DOLT

name detail mean_mult
dolt_blame_basic system table 1.21
dolt_blame_commit_filter system table 2.72
dolt_commit_ancestors_commit_filter system table 0.87
dolt_commits_commit_filter system table 0.97
dolt_diff_log_join_from_commit system table 1.61
dolt_diff_log_join_to_commit system table 1.61
dolt_diff_table_from_commit_filter system table 1.06
dolt_diff_table_to_commit_filter system table 1.13
dolt_diffs_commit_filter system table 1
dolt_history_commit_filter system table 1.23
dolt_log_commit_filter system table 0.93

@Hydrocharged Hydrocharged deleted the aaron/sql-server-startup-rigor branch February 7, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants