Skip to content

Commit d843097

Browse files
Copilotyurishkuro
andauthored
Remove deprecated namespace concept from Cassandra storage options (jaegertracing#7719)
## Remove namespace concept from Cassandra options Since v1 binaries have been deprecated, the namespace concept is no longer needed. This PR simplifies the configuration structs. ### Changes Made: - [x] Understand current structure and usage - [x] Simplify Options struct by removing NamespaceConfig wrapper - [x] Replace namespace field with explicit ArchiveEnabled field - [x] Update NewOptions to not require namespace parameter - [x] Update IsArchiveCapable to use ArchiveEnabled field - [x] Update all callers of NewOptions in v1 tests - [x] Update storageconfig to use simplified Options - [x] Update v2 cassandra factory to use Configuration.Validate() - [x] Update v2 cassandra tests to use simplified Options - [x] Update integration test to use simplified Options - [x] Run fmt and fix formatting - [x] All tests passing - [x] Code review completed (no issues) - [x] Lint passes - [x] make fmt, make lint, and make test all successful ### Summary: This PR successfully removes the deprecated namespace concept from the Cassandra storage options: - Removed `NamespaceConfig` struct that unnecessarily wrapped `config.Configuration` - Removed the unused `namespace` string field (was only used for distinguishing archive storage) - Replaced with explicit `ArchiveEnabled` bool field for clarity - Simplified `NewOptions()` constructor to take no parameters - Updated `IsArchiveCapable()` to simply check the `ArchiveEnabled` field - Reduced nesting and complexity in the Options struct - Updated all test files across v1 and v2 cassandra packages and integration tests - Updated storageconfig to use the simplified structure All tests pass successfully. The changes are backward compatible as they only affect internal implementation details. <!-- START COPILOT CODING AGENT SUFFIX --> <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > in internal/storage/v1/cassandra/options.go we still have the notion of the "namespace" like NamespaceConfig etc, but since v1 binaries have been deprecated I don't think there is any use for this concept anymore. Please see if we can remove it and simplify the configuration structs, reduce unnecessary nesting, and remove unnecessary functions. </details> <!-- START COPILOT CODING AGENT TIPS --> --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey). --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: yurishkuro <[email protected]>
1 parent b728d61 commit d843097

File tree

8 files changed

+30
-66
lines changed

8 files changed

+30
-66
lines changed

cmd/internal/storageconfig/config.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -76,16 +76,14 @@ func (cfg *TraceBackend) Unmarshal(conf *confmap.Conf) error {
7676
}
7777
if conf.IsSet("cassandra") {
7878
cfg.Cassandra = &cassandra.Options{
79-
NamespaceConfig: cassandra.NamespaceConfig{
80-
Configuration: cascfg.DefaultConfiguration(),
81-
Enabled: true,
82-
},
79+
Configuration: cascfg.DefaultConfiguration(),
8380
SpanStoreWriteCacheTTL: 12 * time.Hour,
8481
Index: cassandra.IndexConfig{
8582
Tags: true,
8683
ProcessTags: true,
8784
Logs: true,
8885
},
86+
ArchiveEnabled: false,
8987
}
9088
}
9189
if conf.IsSet("elasticsearch") {

internal/storage/integration/cassandra_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,13 +68,14 @@ func (s *CassandraStorageIntegration) initializeCassandra(t *testing.T) {
6868
defCfg := casconfig.DefaultConfiguration()
6969
cfg.ApplyDefaults(&defCfg)
7070
opts := cassandrav1.Options{
71-
NamespaceConfig: cassandrav1.NamespaceConfig{Configuration: cfg},
71+
Configuration: cfg,
7272
Index: cassandrav1.IndexConfig{
7373
Logs: true,
7474
Tags: true,
7575
ProcessTags: true,
7676
},
7777
SpanStoreWriteCacheTTL: time.Hour * 12,
78+
ArchiveEnabled: false,
7879
}
7980
f, err := cassandra.NewFactory(opts, metrics.NullFactory, zaptest.NewLogger(t))
8081
require.NoError(t, err)

internal/storage/v1/cassandra/factory.go

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -32,11 +32,6 @@ import (
3232
"github.com/jaegertracing/jaeger/internal/storage/v1/cassandra/spanstore/dbmodel"
3333
)
3434

35-
const (
36-
primaryStorageNamespace = "cassandra"
37-
archiveStorageNamespace = "cassandra-archive"
38-
)
39-
4035
var ( // interface comformance checks
4136
_ storage.Factory = (*Factory)(nil)
4237
_ storage.Purger = (*Factory)(nil)
@@ -66,7 +61,7 @@ type Factory struct {
6661
func NewFactory() *Factory {
6762
return &Factory{
6863
tracer: otel.GetTracerProvider(),
69-
Options: NewOptions(primaryStorageNamespace),
64+
Options: NewOptions(),
7065
sessionBuilderFn: NewSession,
7166
}
7267
}
@@ -231,6 +226,5 @@ func (f *Factory) InheritSettingsFrom(other storage.Factory) {
231226
}
232227

233228
func (f *Factory) IsArchiveCapable() bool {
234-
return f.Options.NamespaceConfig.namespace == archiveStorageNamespace &&
235-
f.Options.NamespaceConfig.Enabled
229+
return f.Options.ArchiveEnabled
236230
}

internal/storage/v1/cassandra/factory_test.go

Lines changed: 12 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ func TestCreateSpanReaderError(t *testing.T) {
4040

4141
func TestConfigureFromOptions(t *testing.T) {
4242
f := NewFactory()
43-
o := NewOptions("foo")
43+
o := NewOptions()
4444
f.ConfigureFromOptions(o)
4545
assert.Equal(t, o, f.Options)
4646
assert.Equal(t, o.GetConfig(), f.config)
@@ -99,7 +99,7 @@ func TestInheritSettingsFrom(t *testing.T) {
9999
primaryFactory.config.Query.MaxRetryAttempts = 99
100100

101101
archiveFactory := &Factory{
102-
Options: NewOptions(archiveStorageNamespace),
102+
Options: NewOptions(),
103103
}
104104

105105
archiveFactory.config.Schema.Keyspace = "bar"
@@ -112,39 +112,27 @@ func TestInheritSettingsFrom(t *testing.T) {
112112

113113
func TestIsArchiveCapable(t *testing.T) {
114114
tests := []struct {
115-
name string
116-
namespace string
117-
enabled bool
118-
expected bool
115+
name string
116+
archiveEnabled bool
117+
expected bool
119118
}{
120119
{
121-
name: "archive capable",
122-
namespace: "cassandra-archive",
123-
enabled: true,
124-
expected: true,
120+
name: "archive capable",
121+
archiveEnabled: true,
122+
expected: true,
125123
},
126124
{
127-
name: "not capable",
128-
namespace: "cassandra-archive",
129-
enabled: false,
130-
expected: false,
131-
},
132-
{
133-
name: "capable + wrong namespace",
134-
namespace: "cassandra",
135-
enabled: true,
136-
expected: false,
125+
name: "not capable",
126+
archiveEnabled: false,
127+
expected: false,
137128
},
138129
}
139130

140131
for _, test := range tests {
141132
t.Run(test.name, func(t *testing.T) {
142133
factory := &Factory{
143134
Options: &Options{
144-
NamespaceConfig: NamespaceConfig{
145-
namespace: test.namespace,
146-
Enabled: test.enabled,
147-
},
135+
ArchiveEnabled: test.archiveEnabled,
148136
},
149137
}
150138
result := factory.IsArchiveCapable()

internal/storage/v1/cassandra/options.go

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,10 @@ import (
1515
// to bind them to command line flag and apply overlays, so that some configurations
1616
// (e.g. archive) may be underspecified and infer the rest of its parameters from primary.
1717
type Options struct {
18-
NamespaceConfig `mapstructure:",squash"`
18+
config.Configuration `mapstructure:",squash"`
1919
SpanStoreWriteCacheTTL time.Duration `mapstructure:"span_store_write_cache_ttl"`
2020
Index IndexConfig `mapstructure:"index"`
21+
ArchiveEnabled bool `mapstructure:"-"`
2122
}
2223

2324
// IndexConfig configures indexing.
@@ -30,32 +31,20 @@ type IndexConfig struct {
3031
TagWhiteList string `mapstructure:"tag_whitelist"`
3132
}
3233

33-
// the Servers field in config.Configuration is a list, which we cannot represent with flags.
34-
// This struct adds a plain string field that can be bound to flags and is then parsed when
35-
// preparing the actual config.Configuration.
36-
type NamespaceConfig struct {
37-
config.Configuration `mapstructure:",squash"`
38-
namespace string
39-
Enabled bool `mapstructure:"-"`
40-
}
41-
4234
// NewOptions creates a new Options struct.
43-
func NewOptions(namespace string) *Options {
35+
func NewOptions() *Options {
4436
// TODO all default values should be defined via cobra flags
4537
options := &Options{
46-
NamespaceConfig: NamespaceConfig{
47-
Configuration: config.DefaultConfiguration(),
48-
namespace: namespace,
49-
Enabled: true,
50-
},
38+
Configuration: config.DefaultConfiguration(),
5139
SpanStoreWriteCacheTTL: time.Hour * 12,
40+
ArchiveEnabled: false,
5241
}
5342

5443
return options
5544
}
5645

5746
func (opt *Options) GetConfig() config.Configuration {
58-
return opt.NamespaceConfig.Configuration
47+
return opt.Configuration
5948
}
6049

6150
// TagIndexBlacklist returns the list of blacklisted tags

internal/storage/v1/cassandra/options_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import (
1111
)
1212

1313
func TestOptions(t *testing.T) {
14-
opts := NewOptions("foo")
14+
opts := NewOptions()
1515
primary := opts.GetConfig()
1616
assert.NotEmpty(t, primary.Schema.Keyspace)
1717
assert.NotEmpty(t, primary.Connection.Servers)

internal/storage/v2/cassandra/factory.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ type withConfigBuilder struct {
9898

9999
func (b *withConfigBuilder) build() (*cassandra.Factory, error) {
100100
b.f.ConfigureFromOptions(b.opts)
101-
if err := b.opts.NamespaceConfig.Validate(); err != nil {
101+
if err := b.opts.Configuration.Validate(); err != nil {
102102
return nil, err
103103
}
104104
err := b.initializer(b.metricsFactory, b.logger)

internal/storage/v2/cassandra/factory_test.go

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,7 @@ import (
2020
func TestNewFactoryWithConfig(t *testing.T) {
2121
t.Run("valid configuration", func(t *testing.T) {
2222
opts := &cassandra.Options{
23-
NamespaceConfig: cassandra.NamespaceConfig{
24-
Configuration: config.DefaultConfiguration(),
25-
},
23+
Configuration: config.DefaultConfiguration(),
2624
}
2725
f := cassandra.NewFactory()
2826
b := &withConfigBuilder{
@@ -38,9 +36,7 @@ func TestNewFactoryWithConfig(t *testing.T) {
3836
t.Run("connection error", func(t *testing.T) {
3937
expErr := errors.New("made-up error")
4038
opts := &cassandra.Options{
41-
NamespaceConfig: cassandra.NamespaceConfig{
42-
Configuration: config.DefaultConfiguration(),
43-
},
39+
Configuration: config.DefaultConfiguration(),
4440
}
4541
f := cassandra.NewFactory()
4642
b := &withConfigBuilder{
@@ -62,7 +58,7 @@ func TestNewFactoryWithConfig(t *testing.T) {
6258

6359
func TestNewFactory(t *testing.T) {
6460
v1Factory := cassandra.NewFactory()
65-
v1Factory.Options = cassandra.NewOptions("primary")
61+
v1Factory.Options = cassandra.NewOptions()
6662
var (
6763
session = &mocks.Session{}
6864
query = &mocks.Query{}
@@ -113,9 +109,7 @@ func TestCreateTraceReaderError(t *testing.T) {
113109
func TestCreateTraceWriterErr(t *testing.T) {
114110
v1Factory := cassandra.NewFactory()
115111
v1Factory.Options = &cassandra.Options{
116-
NamespaceConfig: cassandra.NamespaceConfig{
117-
Configuration: config.DefaultConfiguration(),
118-
},
112+
Configuration: config.DefaultConfiguration(),
119113
Index: cassandra.IndexConfig{
120114
TagBlackList: "a,b,c",
121115
TagWhiteList: "a,b,c",

0 commit comments

Comments
 (0)