testserver: set default rf to 1 for single node servers#142108
testserver: set default rf to 1 for single node servers#142108craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @DarrylWong and @herkolategan)
pkg/server/initial_sql.go line 40 at r1 (raw file):
if startSingleNode { // For start-single-node, set the default replication factor to
this code change looks great for TestServer. however, it will change the behavior of cockroach start-single-node, since this code path was the way the RF was being set to 1 for that command.
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/server/initial_sql.go line 40 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
this code change looks great for TestServer. however, it will change the behavior of
cockroach start-single-node, since this code path was the way the RF was being set to 1 for that command.
seems like the test TestDockerCLI_test_disable_replication noticed this too
We already set the default rf to 1 for non system ranges. This patch skips the schema change run to set num replicas for system ranges to 1 and sets the system ranges rf to 1 by default instead. This patch reduces the test runtime of server startup by abour 15%, post initialization: ``` ❯ benchdiff --old 281a729 --new c14e2c15b8697e62a8916cd5c60a1be43c54fdf1 ./pkg/server -r BenchmarkTestServerStartup -c 15 --preview -b old: 281a729 Merge cockroachdb#142062 cockroachdb#142079 new: c14e2c1 testserver: set default rf to 1 for single node se args: benchdiff "--old" "281a7294cf1360446177ceed1e25e141d19e0c6e" "--new" "c14e2c15b8697e62a8916cd5c60a1be43c54fdf1" "./pkg/server" "-r" "BenchmarkTestServerStartup" "-c" "15" "--preview" "-b" name old time/op new time/op delta TestServerStartup/ExternalTenant-24 390ms ±10% 320ms ±26% -18.06% (p=0.000 n=15+15) TestServerStartup/SharedTenant-24 390ms ±16% 328ms ± 7% -15.85% (p=0.000 n=15+14) TestServerStartup/SystemTenantOnly-24 386ms ±13% 336ms ±12% -13.01% (p=0.000 n=15+15) name old alloc/op new alloc/op delta TestServerStartup/SystemTenantOnly-24 133MB ± 0% 119MB ± 0% -10.21% (p=0.000 n=15+15) TestServerStartup/ExternalTenant-24 133MB ± 0% 119MB ± 1% -10.19% (p=0.000 n=12+15) TestServerStartup/SharedTenant-24 133MB ± 1% 119MB ± 1% -10.18% (p=0.000 n=15+14) name old allocs/op new allocs/op delta TestServerStartup/SystemTenantOnly-24 799k ± 0% 701k ± 0% -12.34% (p=0.000 n=15+15) TestServerStartup/SharedTenant-24 799k ± 0% 701k ± 0% -12.33% (p=0.000 n=15+15) TestServerStartup/ExternalTenant-24 799k ± 0% 701k ± 0% -12.29% (p=0.000 n=15+15) ``` Epic: none Release note: none
1f21019 to
b30f967
Compare
msbutler
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/server/initial_sql.go line 40 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
seems like the test
TestDockerCLI_test_disable_replicationnoticed this too
whoops! fixed.
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
lgtm!
|
TFTR! bors r=rafiss |
We already set the default rf to 1 for non system ranges. This patch skips a schema change run to set num replicas for system ranges to 1 and sets the system ranges rf to 1 by default instead. This patch reduces the test runtime of server startup by abour 15%, post initialization:
Epic: none
Release note: none