Skip to content
This repository has been archived by the owner on Apr 3, 2024. It is now read-only.

Set dynamic config values and default search attribute cache as disabled #136

Merged
merged 4 commits into from
Sep 23, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 52 additions & 12 deletions cmd/temporalite/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package main

import (
"encoding/json"
"fmt"
goLog "log"
"net"
Expand All @@ -13,6 +14,7 @@ import (

"github.com/urfave/cli/v2"
"go.temporal.io/server/common/config"
"go.temporal.io/server/common/dynamicconfig"
"go.temporal.io/server/common/headers"
"go.temporal.io/server/common/log"
"go.temporal.io/server/temporal"
Expand All @@ -35,18 +37,19 @@ var (
)

const (
ephemeralFlag = "ephemeral"
dbPathFlag = "filename"
portFlag = "port"
metricsPortFlag = "metrics-port"
uiPortFlag = "ui-port"
headlessFlag = "headless"
ipFlag = "ip"
logFormatFlag = "log-format"
logLevelFlag = "log-level"
namespaceFlag = "namespace"
pragmaFlag = "sqlite-pragma"
configFlag = "config"
ephemeralFlag = "ephemeral"
dbPathFlag = "filename"
portFlag = "port"
metricsPortFlag = "metrics-port"
uiPortFlag = "ui-port"
headlessFlag = "headless"
ipFlag = "ip"
logFormatFlag = "log-format"
logLevelFlag = "log-level"
namespaceFlag = "namespace"
pragmaFlag = "sqlite-pragma"
configFlag = "config"
dynamicConfigValueFlag = "dynamic-config-value"
)

func init() {
Expand Down Expand Up @@ -146,6 +149,10 @@ func buildCLI() *cli.App {
EnvVars: []string{config.EnvKeyConfigDir},
Value: "",
},
&cli.StringSliceFlag{
Name: dynamicConfigValueFlag,
Usage: `dynamic config value, as KEY=JSON_VALUE (meaning strings need quotes)`,
},
jlegrone marked this conversation as resolved.
Show resolved Hide resolved
},
Before: func(c *cli.Context) error {
if c.Args().Len() > 0 {
Expand Down Expand Up @@ -262,6 +269,21 @@ func buildCLI() *cli.App {
}
opts = append(opts, temporalite.WithLogger(logger))

configVals, err := getDynamicConfigValues(c.StringSlice(dynamicConfigValueFlag))
if err != nil {
return err
}
// If there is no config value for search attribute cache disabling,
// default it to true
cretz marked this conversation as resolved.
Show resolved Hide resolved
if len(configVals[dynamicconfig.ForceSearchAttributesCacheRefreshOnRead]) == 0 {
configVals[dynamicconfig.ForceSearchAttributesCacheRefreshOnRead] = []dynamicconfig.ConstrainedValue{
{Value: true},
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This further cements that Temporalite is not for production use.

I'd love to ship "production ready" defaults as much as possible. Do we think that disabling this cache will be a limiting factor in terms of performance? Trading a small amount of runtime performance for no race conditions or startup wait time sounds fine to me, though we could also consider only enabling this in the temporaltest package if the runtime cost is high.

Copy link
Collaborator

@jlegrone jlegrone Sep 22, 2022

Choose a reason for hiding this comment

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

Also instead of setting a default here, I think we should probably do it inside temporalite.NewServer so that library users get the same behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

I originally had the same concerns but thought Temporalite was going for "development defaults" even in the CLI. I would be happy only setting this in temporaltest. Let me confirm with @bergundy who wanted this defauted in the CLI.

Copy link
Member

Choose a reason for hiding this comment

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

This makes me think that having the temporalite "yolo" command for common test setup (includes --ephemeral too) is starting to make sense.

Copy link
Collaborator

@jlegrone jlegrone Sep 22, 2022

Choose a reason for hiding this comment

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

IMO Temporalite values go something like this

  1. User experience (for development use cases)
  2. Security
  3. Performance
  4. Simplicity
  5. Configurability

So I'm very happy to trade off some performance for a better development experience. We can always add a --production flag to swap around a few defaults and keep things simple.

Another thought is that we could add flags to register search attributes at startup like we do for namespaces. This would essentially be the best of both worlds since we'd be able to ensure that each of those SAs is registered synchronously, without disabling caching completely.

Copy link
Member

Choose a reason for hiding this comment

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

Another thought is that we could add flags to register search attributes at startup like we do for namespaces. This would essentially be the best of both worlds since we'd be able to ensure that each of those SAs is registered synchronously, without disabling caching completely.

I'd rather users do this with the SDK so their tests can run against any server implementation without customization.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed default from CLI

for k, v := range configVals {
opts = append(opts, temporalite.WithDynamicConfigValue(k, v))
}

s, err := temporalite.NewServer(opts...)
if err != nil {
return err
Expand Down Expand Up @@ -289,3 +311,21 @@ func getPragmaMap(input []string) (map[string]string, error) {
}
return result, nil
}

func getDynamicConfigValues(input []string) (map[dynamicconfig.Key][]dynamicconfig.ConstrainedValue, error) {
ret := make(map[dynamicconfig.Key][]dynamicconfig.ConstrainedValue, len(input))
for _, keyValStr := range input {
keyVal := strings.SplitN(keyValStr, "=", 2)
if len(keyVal) != 2 {
return nil, fmt.Errorf("dynamic config value not in KEY=JSON_VAL format")
}
key := dynamicconfig.Key(keyVal[0])
jlegrone marked this conversation as resolved.
Show resolved Hide resolved
// We don't support constraints currently
Copy link
Collaborator

Choose a reason for hiding this comment

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

Constraints sound nice... what are they and is there future work we should track to support them?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really know, I'd have to speak to the server team. My use case and most use cases I've seen don't have these constraints. We can block this PR for that if we really need to.

var val dynamicconfig.ConstrainedValue
if err := json.Unmarshal([]byte(keyVal[1]), &val.Value); err != nil {
return nil, fmt.Errorf("invalid JSON value for key %q: %w", key, err)
}
ret[key] = append(ret[key], val)
}
return ret, nil
}
45 changes: 45 additions & 0 deletions cmd/temporalite/main_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License.
cretz marked this conversation as resolved.
Show resolved Hide resolved
//
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2021 Datadog, Inc.

package main

import (
"reflect"
"testing"
)

func TestGetDynamicConfigValues(t *testing.T) {
assertBadVal := func(v string) {
if _, err := getDynamicConfigValues([]string{v}); err == nil {
t.Fatalf("expected error for %v", v)
}
}
type v map[string][]interface{}
assertGoodVals := func(expected v, in ...string) {
actualVals, err := getDynamicConfigValues(in)
if err != nil {
t.Fatal(err)
}
actual := make(v, len(actualVals))
for k, vals := range actualVals {
for _, val := range vals {
actual[string(k)] = append(actual[string(k)], val.Value)
}
}
if !reflect.DeepEqual(expected, actual) {
t.Fatalf("not equal, expected - actual: %v - %v", expected, actual)
}
}

assertBadVal("foo")
assertBadVal("foo=")
assertBadVal("foo=bar")
assertBadVal("foo=123a")

assertGoodVals(v{"foo": {123.0}}, "foo=123")
assertGoodVals(
v{"foo": {123.0, []interface{}{"123", false}}, "bar": {"baz"}, "qux": {true}},
"foo=123", `bar="baz"`, "qux=true", `foo=["123", false]`,
jlegrone marked this conversation as resolved.
Show resolved Hide resolved
)
}
2 changes: 2 additions & 0 deletions internal/liteconfig/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (

"go.temporal.io/server/common/cluster"
"go.temporal.io/server/common/config"
"go.temporal.io/server/common/dynamicconfig"
"go.temporal.io/server/common/log"
"go.temporal.io/server/common/metrics"
"go.temporal.io/server/common/persistence/sql/sqlplugin/sqlite"
Expand Down Expand Up @@ -59,6 +60,7 @@ type Config struct {
FrontendIP string
UIServer UIServer
BaseConfig *config.Config
DynamicConfig dynamicconfig.StaticClient
}

var SupportedPragmas = map[string]struct{}{
Expand Down
21 changes: 21 additions & 0 deletions options.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package temporalite

import (
"go.temporal.io/server/common/config"
"go.temporal.io/server/common/dynamicconfig"
"go.temporal.io/server/common/log"
"go.temporal.io/server/temporal"

Expand Down Expand Up @@ -119,6 +120,26 @@ func WithBaseConfig(base *config.Config) ServerOption {
})
}

// WithDynamicConfigValue sets the given dynamic config key with the given set
// of values. This will overwrite the key if already set.
func WithDynamicConfigValue(key dynamicconfig.Key, value []dynamicconfig.ConstrainedValue) ServerOption {
return newApplyFuncContainer(func(cfg *liteconfig.Config) {
if cfg.DynamicConfig == nil {
cfg.DynamicConfig = dynamicconfig.StaticClient{}
}
cfg.DynamicConfig[key] = value
})
}

// WithSearchAttributeCacheDisabled disables search attribute caching. This
// delegates to WithDynamicConfigValue.
func WithSearchAttributeCacheDisabled() ServerOption {
return WithDynamicConfigValue(
dynamicconfig.ForceSearchAttributesCacheRefreshOnRead,
[]dynamicconfig.ConstrainedValue{{Value: true}},
)
}
Comment on lines +134 to +141
Copy link
Collaborator

Choose a reason for hiding this comment

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

Commented above, but should we make this the default and instead add an option for enabling the cache? We could comment to the effect of changing this default if/when we are able to remove the waiting period after registering search attributes, and if you need stable behavior use WithDynamicConfigValue.

Copy link
Member Author

@cretz cretz Sep 22, 2022

Choose a reason for hiding this comment

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

I think it should be opt-in programmatically, like persistence-disabled, especially now that we are going to turn it off by default on the CLI. (will still use in temporaltest)


type applyFuncContainer struct {
applyInternal func(*liteconfig.Config)
}
Expand Down
11 changes: 9 additions & 2 deletions server.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
"go.temporal.io/sdk/client"
"go.temporal.io/server/common/authorization"
"go.temporal.io/server/common/config"
"go.temporal.io/server/common/dynamicconfig"
"go.temporal.io/server/schema/sqlite"
"go.temporal.io/server/temporal"

Expand Down Expand Up @@ -94,7 +93,15 @@ func NewServer(opts ...ServerOption) (*Server, error) {
temporal.WithClaimMapper(func(cfg *config.Config) authorization.ClaimMapper {
return claimMapper
}),
temporal.WithDynamicConfigClient(dynamicconfig.NewNoopClient()),
}

if len(c.DynamicConfig) > 0 {
// To prevent having to code fall-through semantics right now, we currently
// eagerly fail if dynamic config is being configured in two ways
if cfg.DynamicConfigClient != nil {
return nil, fmt.Errorf("unable to have file-based dynamic config and individual dynamic config values")
}
serverOpts = append(serverOpts, temporal.WithDynamicConfigClient(c.DynamicConfig))
}

if len(c.UpstreamOptions) > 0 {
Expand Down
1 change: 1 addition & 0 deletions temporaltest/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ func NewServer(opts ...TestServerOption) *TestServer {
temporalite.WithPersistenceDisabled(),
temporalite.WithDynamicPorts(),
temporalite.WithLogger(log.NewNoopLogger()),
temporalite.WithSearchAttributeCacheDisabled(),
)

s, err := temporalite.NewServer(ts.serverOptions...)
Expand Down
28 changes: 27 additions & 1 deletion temporaltest/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import (
"testing"
"time"

"go.temporal.io/api/enums/v1"
"go.temporal.io/api/operatorservice/v1"
"go.temporal.io/sdk/client"
"go.temporal.io/sdk/worker"

Expand Down Expand Up @@ -134,7 +136,6 @@ func TestDefaultWorkerOptions(t *testing.T) {
ts.NewWorker("hello_world", func(registry worker.Registry) {
helloworld.RegisterWorkflowsAndActivities(registry)
})

ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()

Expand Down Expand Up @@ -196,6 +197,31 @@ func TestClientWithDefaultInterceptor(t *testing.T) {
}
}

func TestSearchAttributeCacheDisabled(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
ts := temporaltest.NewServer(temporaltest.WithT(t))

// Create a search attribute
_, err := ts.DefaultClient().OperatorService().AddSearchAttributes(ctx, &operatorservice.AddSearchAttributesRequest{
SearchAttributes: map[string]enums.IndexedValueType{
"my-search-attr": enums.INDEXED_VALUE_TYPE_TEXT,
},
})
if err != nil {
t.Fatal(err)
}

// Confirm it exists immediately
resp, err := ts.DefaultClient().GetSearchAttributes(ctx)
if err != nil {
t.Fatal(err)
}
if resp.Keys["my-search-attr"] != enums.INDEXED_VALUE_TYPE_TEXT {
t.Fatal("search attribute not found")
}
}

func BenchmarkRunWorkflow(b *testing.B) {
ts := temporaltest.NewServer()
defer ts.Stop()
Expand Down