-
Notifications
You must be signed in to change notification settings - Fork 344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Cassandra create-schema job #71
Add support for Cassandra create-schema job #71
Conversation
Missing:
|
@objectiser , @pavolloffay would one of you like to review this? |
Codecov Report
@@ Coverage Diff @@
## master #71 +/- ##
==========================================
+ Coverage 99.3% 99.37% +0.07%
==========================================
Files 17 19 +2
Lines 718 801 +83
==========================================
+ Hits 713 796 +83
Misses 5 5
Continue to review full report at Codecov.
|
@jpkrohling As a general comment, I think this has thrown up an issue for me with the use of Ideally I think the schema creation config should be a part of the overall cassandra config, e.g.
and I assume having the cassandra config statically typed would identify errors in the CR, rather than waiting for the Jaeger instance to be deployed and throw errors due to the passed parameters being incorrect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are settings for TTL, replication and keyspace?
|
||
// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new JaegerCassandraCreateSchemaSpec. | ||
func (in *JaegerCassandraCreateSchemaSpec) DeepCopy() *JaegerCassandraCreateSchemaSpec { | ||
if in == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in CopyInto this check is missing. Is it a convenience? They both could define the same behavior and preconditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is generated code
They are missing on this first PR, but I'll probably add them before removing the WIP flag. |
65fcb75
to
94d9bbf
Compare
@pavolloffay, @objectiser I just added a new commit with tests. I believe this is ready to be reviewed. |
|
||
=== Cassandra | ||
|
||
When the storage type is set to Cassandra, the operator will automatically create a batch job that creates the required schema for Jaeger to run. This batch job will block the Jaeger installation, so that it starts only after the schema is successfuly created. The creation of this batch job can be disabled by setting the `enabled` property to `false`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering if it would be safer to default to false? If someone is using an existing jaeger cassandra storage - what is the impact of the job running, is it a noop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current scripts are noop, as they have an "if not exists" clause:
But it might be a good idea to add a flag to the binary, to get this enabled/disabled by default, as we discussed today in the meeting. Something for a follow up PR, to be discussed with the ES parts?
test/e2e/wait_util.go
Outdated
@@ -87,3 +87,29 @@ func WaitForIngress(t *testing.T, kubeclient kubernetes.Interface, namespace, na | |||
t.Logf("Ingress available\n") | |||
return nil | |||
} | |||
|
|||
// WaitForJob checks to see if a given job has the completed successfuly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove 'the'
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
94d9bbf
to
090da1a
Compare
This PR adds support for the Cassandra create-schema job, backed by the create-schema script.
With this PR, the following template can be used:
I'm not 100% happy with the "datanceter" option duplicating part of the "keyspace" property from the
options
object, but that's how they are also today. Perhaps we can improve the create-schema script to not require a datacenter/mode, accepting only a "keyspace".Signed-off-by: Juraci Paixão Kröhling [email protected]