Skip to content

Commit 4a6fae2

Browse files
ecordellecordell
authored
Merge pull request #2300 from ecordell/rrflags
support both old and new read replica flag prefixes
2 parents eb99ce9 + c523369 commit 4a6fae2

File tree

4 files changed

+169
-4
lines changed

4 files changed

+169
-4
lines changed

‎pkg/cmd/datastore/datastore.go

+20-2
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,9 @@ type Config struct {
118118
IncludeQueryParametersInTraces bool `debugmap:"visible"`
119119

120120
// Read Replicas
121-
ReadReplicaConnPool ConnPoolConfig `debugmap:"visible"`
121+
ReadReplicaConnPool ConnPoolConfig `debugmap:"visible"`
122+
// this holds values from the old flag prefix in case they are used
123+
OldReadReplicaConnPool ConnPoolConfig `debugmap:"hidden"`
122124
ReadReplicaURIs []string `debugmap:"sensitive"`
123125
ReadReplicaCredentialsProviderName string `debugmap:"visible"`
124126

@@ -211,7 +213,22 @@ func RegisterDatastoreFlagsWithPrefix(flagSet *pflag.FlagSet, prefix string, opt
211213
deprecateUnifiedConnFlags(flagSet)
212214
RegisterConnPoolFlagsWithPrefix(flagSet, "datastore-conn-pool-read", &legacyConnPool, &opts.ReadConnPool)
213215
RegisterConnPoolFlagsWithPrefix(flagSet, "datastore-conn-pool-write", DefaultWriteConnPool(), &opts.WriteConnPool)
214-
RegisterConnPoolFlagsWithPrefix(flagSet, "datastore-read-replica-conn-pool-read", DefaultReadConnPool(), &opts.ReadReplicaConnPool)
216+
217+
// read replica prefix changed but we retain backward-compatibility
218+
newReadReplicaPrefix := "datastore-read-replica-conn-pool-read"
219+
oldReadReplicaPrefix := "datastore-read-replica-conn-pool"
220+
RegisterConnPoolFlagsWithPrefix(flagSet, newReadReplicaPrefix, DefaultReadConnPool(), &opts.ReadReplicaConnPool)
221+
RegisterConnPoolFlagsWithPrefix(flagSet, oldReadReplicaPrefix, DefaultReadConnPool(), &opts.OldReadReplicaConnPool)
222+
223+
warning := fmt.Sprintf("please use the flags with the prefix %q instead of %q", newReadReplicaPrefix, oldReadReplicaPrefix)
224+
for _, flag := range []string{"max-open", "min-open", "max-lifetime", "max-lifetime-jitter", "max-idletime", "healthcheck-interval"} {
225+
if err := flagSet.MarkDeprecated(oldReadReplicaPrefix+"-"+flag, warning); err != nil {
226+
return fmt.Errorf("failed to mark flag as deprecated: %w", err)
227+
}
228+
if err := flagSet.MarkHidden(oldReadReplicaPrefix + "-" + flag); err != nil {
229+
return fmt.Errorf("failed to mark flag as hidden: %w", err)
230+
}
231+
}
215232

216233
normalizeFunc := flagSet.GetNormalizeFunc()
217234
flagSet.SetNormalizeFunc(func(f *pflag.FlagSet, name string) pflag.NormalizedName {
@@ -296,6 +313,7 @@ func DefaultDatastoreConfig() *Config {
296313
ReadConnPool: *DefaultReadConnPool(),
297314
WriteConnPool: *DefaultWriteConnPool(),
298315
ReadReplicaConnPool: *DefaultReadConnPool(),
316+
OldReadReplicaConnPool: *DefaultReadConnPool(),
299317
ReadReplicaURIs: []string{},
300318
ReadOnly: false,
301319
MaxRetries: 10,

‎pkg/cmd/datastore/zz_generated.options.go

+8
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/cmd/server/server.go

+44-2
Original file line numberDiff line numberDiff line change
@@ -192,8 +192,6 @@ func (c *closeableStack) CloseIfError(err error) error {
192192
// if there is no error, a completedServerConfig (with limited options for
193193
// mutation) is returned.
194194
func (c *Config) Complete(ctx context.Context) (RunnableServer, error) {
195-
log.Ctx(ctx).Info().Fields(helpers.Flatten(c.DebugMap())).Msg("configuration")
196-
197195
closeables := closeableStack{}
198196
var err error
199197
defer func() {
@@ -225,6 +223,7 @@ func (c *Config) Complete(ctx context.Context) (RunnableServer, error) {
225223
ds := c.Datastore
226224
if ds == nil {
227225
var err error
226+
c.supportOldAndNewReadReplicaConnectionPoolFlags()
228227
ds, err = datastorecfg.NewDatastore(context.Background(), c.DatastoreConfig.ToOption(),
229228
// Datastore's filter maximum ID count is set to the max size, since the number of elements to be dispatched
230229
// are at most the number of elements returned from a datastore query
@@ -513,6 +512,8 @@ func (c *Config) Complete(ctx context.Context) (RunnableServer, error) {
513512
}
514513
closeables.AddWithoutError(metricsServer.Close)
515514

515+
log.Ctx(ctx).Info().Fields(helpers.Flatten(c.DebugMap())).Msg("configuration")
516+
516517
return &completedServerConfig{
517518
ds: ds,
518519
gRPCServer: grpcServer,
@@ -528,6 +529,47 @@ func (c *Config) Complete(ctx context.Context) (RunnableServer, error) {
528529
}, nil
529530
}
530531

532+
func (c *Config) supportOldAndNewReadReplicaConnectionPoolFlags() {
533+
defaultReadConnPoolCfg := *datastorecfg.DefaultReadConnPool()
534+
if c.DatastoreConfig.ReadReplicaConnPool.MaxOpenConns == defaultReadConnPoolCfg.MaxOpenConns && c.DatastoreConfig.
535+
OldReadReplicaConnPool.MaxOpenConns != defaultReadConnPoolCfg.MaxOpenConns {
536+
c.DatastoreConfig.
537+
ReadReplicaConnPool.MaxOpenConns = c.DatastoreConfig.
538+
OldReadReplicaConnPool.MaxOpenConns
539+
}
540+
if c.DatastoreConfig.
541+
ReadReplicaConnPool.MinOpenConns == defaultReadConnPoolCfg.MinOpenConns && c.DatastoreConfig.
542+
OldReadReplicaConnPool.MinOpenConns != defaultReadConnPoolCfg.MinOpenConns {
543+
c.DatastoreConfig.
544+
ReadReplicaConnPool.MinOpenConns = c.DatastoreConfig.
545+
OldReadReplicaConnPool.MinOpenConns
546+
}
547+
if c.DatastoreConfig.
548+
ReadReplicaConnPool.MaxLifetime == defaultReadConnPoolCfg.MaxLifetime && c.DatastoreConfig.
549+
OldReadReplicaConnPool.MaxLifetime != defaultReadConnPoolCfg.MaxLifetime {
550+
c.DatastoreConfig.
551+
ReadReplicaConnPool.MaxLifetime = c.DatastoreConfig.
552+
OldReadReplicaConnPool.MaxLifetime
553+
}
554+
if c.DatastoreConfig.
555+
ReadReplicaConnPool.MaxLifetimeJitter == defaultReadConnPoolCfg.MaxLifetimeJitter && c.DatastoreConfig.
556+
OldReadReplicaConnPool.MaxLifetimeJitter != defaultReadConnPoolCfg.MaxLifetimeJitter {
557+
c.DatastoreConfig.
558+
ReadReplicaConnPool.MaxLifetimeJitter = c.DatastoreConfig.
559+
OldReadReplicaConnPool.MaxLifetimeJitter
560+
}
561+
if c.DatastoreConfig.
562+
ReadReplicaConnPool.MaxIdleTime == defaultReadConnPoolCfg.MaxIdleTime && c.DatastoreConfig.
563+
OldReadReplicaConnPool.MaxIdleTime != defaultReadConnPoolCfg.MaxIdleTime {
564+
c.DatastoreConfig.
565+
ReadReplicaConnPool.MaxIdleTime = c.DatastoreConfig.OldReadReplicaConnPool.MaxIdleTime
566+
}
567+
if c.DatastoreConfig.ReadReplicaConnPool.HealthCheckInterval == defaultReadConnPoolCfg.HealthCheckInterval &&
568+
c.DatastoreConfig.OldReadReplicaConnPool.HealthCheckInterval != defaultReadConnPoolCfg.HealthCheckInterval {
569+
c.DatastoreConfig.ReadReplicaConnPool.HealthCheckInterval = c.DatastoreConfig.OldReadReplicaConnPool.HealthCheckInterval
570+
}
571+
}
572+
531573
func (c *Config) buildUnaryMiddleware(defaultMiddleware *MiddlewareChain[grpc.UnaryServerInterceptor]) ([]grpc.UnaryServerInterceptor, error) {
532574
chain := MiddlewareChain[grpc.UnaryServerInterceptor]{}
533575
if defaultMiddleware != nil {

‎pkg/cmd/server/server_test.go

+97
Original file line numberDiff line numberDiff line change
@@ -272,3 +272,100 @@ func TestModifyStreamingMiddleware(t *testing.T) {
272272
err = streaming[1](context.Background(), nil, nil, nil)
273273
require.ErrorContains(t, err, "hi")
274274
}
275+
276+
func TestSupportOldAndNewReadReplicaConnectionPoolFlags(t *testing.T) {
277+
tests := []struct {
278+
name string
279+
opts Config
280+
expected datastore.ConnPoolConfig
281+
}{
282+
{
283+
name: "no flags set",
284+
opts: Config{
285+
DatastoreConfig: datastore.Config{
286+
ReadReplicaConnPool: *datastore.DefaultReadConnPool(),
287+
OldReadReplicaConnPool: *datastore.DefaultReadConnPool(),
288+
},
289+
},
290+
expected: *datastore.DefaultReadConnPool(),
291+
},
292+
{
293+
name: "new flags set",
294+
opts: Config{
295+
DatastoreConfig: datastore.Config{
296+
ReadReplicaConnPool: datastore.ConnPoolConfig{
297+
MaxLifetime: 1 * time.Minute,
298+
MaxIdleTime: 1 * time.Minute,
299+
MaxOpenConns: 1,
300+
MinOpenConns: 1,
301+
HealthCheckInterval: 1 * time.Second,
302+
},
303+
OldReadReplicaConnPool: *datastore.DefaultReadConnPool(),
304+
},
305+
},
306+
expected: datastore.ConnPoolConfig{
307+
MaxLifetime: 1 * time.Minute,
308+
MaxIdleTime: 1 * time.Minute,
309+
MaxOpenConns: 1,
310+
MinOpenConns: 1,
311+
HealthCheckInterval: 1 * time.Second,
312+
},
313+
},
314+
{
315+
name: "old flags set",
316+
opts: Config{
317+
DatastoreConfig: datastore.Config{
318+
ReadReplicaConnPool: *datastore.DefaultReadConnPool(),
319+
OldReadReplicaConnPool: datastore.ConnPoolConfig{
320+
MaxLifetime: 2 * time.Minute,
321+
MaxIdleTime: 2 * time.Minute,
322+
MaxOpenConns: 2,
323+
MinOpenConns: 2,
324+
HealthCheckInterval: 2 * time.Second,
325+
},
326+
},
327+
},
328+
expected: datastore.ConnPoolConfig{
329+
MaxLifetime: 2 * time.Minute,
330+
MaxIdleTime: 2 * time.Minute,
331+
MaxOpenConns: 2,
332+
MinOpenConns: 2,
333+
HealthCheckInterval: 2 * time.Second,
334+
},
335+
},
336+
{
337+
name: "prefers new flags if both are set",
338+
opts: Config{
339+
DatastoreConfig: datastore.Config{
340+
ReadReplicaConnPool: datastore.ConnPoolConfig{
341+
MaxLifetime: 1 * time.Minute,
342+
MaxIdleTime: 1 * time.Minute,
343+
MaxOpenConns: 1,
344+
MinOpenConns: 2,
345+
HealthCheckInterval: 2 * time.Second,
346+
},
347+
OldReadReplicaConnPool: datastore.ConnPoolConfig{
348+
MaxLifetime: 2 * time.Minute,
349+
MaxIdleTime: 2 * time.Minute,
350+
MaxOpenConns: 2,
351+
MinOpenConns: 2,
352+
HealthCheckInterval: 2 * time.Second,
353+
},
354+
},
355+
},
356+
expected: datastore.ConnPoolConfig{
357+
MaxLifetime: 1 * time.Minute,
358+
MaxIdleTime: 1 * time.Minute,
359+
MaxOpenConns: 1,
360+
MinOpenConns: 2,
361+
HealthCheckInterval: 2 * time.Second,
362+
},
363+
},
364+
}
365+
for _, tt := range tests {
366+
t.Run(tt.name, func(t *testing.T) {
367+
tt.opts.supportOldAndNewReadReplicaConnectionPoolFlags()
368+
require.Equal(t, tt.expected, tt.opts.DatastoreConfig.ReadReplicaConnPool)
369+
})
370+
}
371+
}

0 commit comments

Comments
 (0)