Create Cassandra db schema on session initialization #5922
Conversation
…ution for initializing database. Signed-off-by: Alok Kumar Singh <[email protected]>
| @@ -131,6 +137,262 @@ func (f *Factory) configureFromOptions(o *Options) { | |||
| } | |||
| } | |||
|
|
|||
| const ( | |||
There was a problem hiding this comment.
please move the changes to a separate file, like schema.go, there's no reason for this to be in factory.go
| REPLICATION_FACTOR = `REPLICATION_FACTOR` | ||
| VERSION = `VERSION` | ||
| COMPACTION_WINDOW = `COMPACTION_WINDOW` | ||
| ) |
There was a problem hiding this comment.
The functionality you're adding is only needed in jaeger-v2, which uses YAML config files. So there will be no env variables - you can simply use the struct below. But you should add mapstructure tags to the fields, as well as validation instructions (example: cmd/jaeger/internal/extension/jaegerquery/config.go)
| datacenter string | ||
| trace_ttl int | ||
| dependencies_ttl int | ||
| keyspace string |
There was a problem hiding this comment.
this should already be defined in the main config
There was a problem hiding this comment.
Are you talking about this config: jaeger/cmd/jaeger/config-cassandra.yaml ? How would I know what all config key-value pairs are already defined?
There was a problem hiding this comment.
Hi @yurishkuro , I needed some help here. Is this: jaeger/cmd/jaeger/internal/all-in-one.yaml the example for the config file which would be used in v2? If yes, under what section should I expect the cassandra specific configs to be put? I couldn't find any example for this in the current code base.
Just to get the complete picture, would the schema configs I am adding look something like this:
schema:
datacenter: <datacenterName>
keyspace: <keyspace>
.......
There was a problem hiding this comment.
cmd/jaeger/config-cassandra.yaml
| ) | ||
|
|
||
| // Parameters required for initilizing the db | ||
| type StorageConfigParams struct { |
| var replication_factor, compaction_window_size int | ||
| var replication, compaction_window_unit string | ||
|
|
||
| mode := os.Getenv(MODE) |
There was a problem hiding this comment.
do we actually need the "mode"?
There was a problem hiding this comment.
In v1 I suppose the mode env variable is used for simple testing scenarios, that's why I added it in this as well. Do you suggest removing it?
| //go:embed schema/v002.cql.tmpl | ||
| //go:embed schema/v003.cql.tmpl | ||
| //go:embed schema/v004.cql.tmpl | ||
| var schemaFile embed.FS |
There was a problem hiding this comment.
why do we need to embed all versions? we keep them mostly for historical reasons, but only the latest is used.
| var schemaFile embed.FS | ||
|
|
||
| func handleTemplateReplacements(data []byte, params *StorageConfigParams) []byte { | ||
| templateKeysValuePairs := map[string]string{ |
There was a problem hiding this comment.
It's better to rewrite the schema with Go template syntax and use that, because then the data binding can be directly against the config struct (better type safety).
| return result | ||
| } | ||
|
|
||
| func constructQueriesFromTemplateFiles(session cassandra.Session, params *StorageConfigParams) ([]cassandra.Query, error) { |
There was a problem hiding this comment.
Is cassandra.Session not able to execute multiple queries at once?
| @@ -143,12 +405,21 @@ func (f *Factory) Initialize(metricsFactory metrics.Factory, logger *zap.Logger) | |||
| } | |||
| f.primarySession = primarySession | |||
|
|
|||
| // After creating a session, execute commands to initialize the setup if not already present | |||
| if err := f.InitializeDB(primarySession); err != nil { | |||
There was a problem hiding this comment.
what happens if the schema already exists?
| ) | ||
|
|
||
| // Parameters required for initilizing the db | ||
| type StorageConfigParams struct { |
There was a problem hiding this comment.
@yurishkuro @akstron i'm working on a proposal to redefine the configs for cassandra in #5928 (comment). What are both of your thoughts on adding the configurations here into a mapstructure:"schema" grouping?
There was a problem hiding this comment.
Do you think schema and data are different?
There was a problem hiding this comment.
Sorry I should've mentioned. I was suggesting consolidating data and schema into one under schema.
…ution for initializing database.
Which problem is this PR solving?
Resolves #5797
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test