Skip to content
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

Cassandra init #6072

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

hellspawn679
Copy link
Contributor

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

mehul and others added 5 commits October 7, 2024 22:52
Signed-off-by: mehul <mehulsharam4786@gmail.com>
fix
Signed-off-by: mehul <mehulsharam4786@gmail.com>
Signed-off-by: mehul <mehulsharam4786@gmail.com>
Signed-off-by: mehul <mehulsharam4786@gmail.com>
@hellspawn679 hellspawn679 requested a review from a team as a code owner October 7, 2024 20:46
@dosubot dosubot bot added the area/storage label Oct 7, 2024
Copy link

codecov bot commented Oct 7, 2024

Codecov Report

Attention: Patch coverage is 71.75573% with 37 lines in your changes missing coverage. Please review.

Project coverage is 96.72%. Comparing base (3551cf7) to head (3aa0941).

Files with missing lines Patch % Lines
plugin/storage/cassandra/schema/create.go 71.53% 27 Missing and 10 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6072      +/-   ##
==========================================
- Coverage   96.92%   96.72%   -0.20%     
==========================================
  Files         349      350       +1     
  Lines       16598    16729     +131     
==========================================
+ Hits        16087    16181      +94     
- Misses        328      355      +27     
- Partials      183      193      +10     
Flag Coverage Δ
badger_v1 7.86% <0.00%> (-0.13%) ⬇️
badger_v2 1.79% <0.00%> (-0.03%) ⬇️
cassandra-4.x-v1 15.81% <17.55%> (+0.02%) ⬆️
cassandra-4.x-v2 1.72% <0.00%> (-0.03%) ⬇️
cassandra-5.x-v1 15.81% <17.55%> (+0.02%) ⬆️
cassandra-5.x-v2 1.72% <0.00%> (-0.03%) ⬇️
elasticsearch-6.x-v1 18.40% <0.00%> (-0.29%) ⬇️
elasticsearch-7.x-v1 18.46% <0.00%> (-0.30%) ⬇️
elasticsearch-8.x-v1 18.64% <0.00%> (-0.31%) ⬇️
elasticsearch-8.x-v2 1.79% <0.00%> (-0.03%) ⬇️
grpc_v1 9.22% <0.00%> (-0.14%) ⬇️
grpc_v2 7.00% <0.00%> (-0.13%) ⬇️
kafka-v1 9.54% <0.00%> (-0.16%) ⬇️
kafka-v2 1.79% <0.00%> (-0.03%) ⬇️
memory_v2 1.79% <0.00%> (-0.02%) ⬇️
opensearch-1.x-v1 18.50% <0.00%> (-0.30%) ⬇️
opensearch-2.x-v1 18.51% <0.00%> (-0.28%) ⬇️
opensearch-2.x-v2 1.79% <0.00%> (-0.02%) ⬇️
tailsampling-processor 0.45% <0.00%> (-0.01%) ⬇️
unittests 95.46% <64.12%> (-0.25%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

}
var datacentre, replications string
// var ReplicationFactor int
if mode == "prod" {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this distinction anymore. Users can provide precise values for parameters, we don't have to have Jaeger guess those parameters based on the MODE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so should i remove the mode attribute and take prod mode as default
?

Copy link
Member

Choose a reason for hiding this comment

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

yes. Whichever parameters the prod attribute affects today should simply be exposed as configuration options for the user.

Copy link
Member

Choose a reason for hiding this comment

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

btw, we do not need to enable this schema-management capability in jaeger-v1, especially if we need new configuration for it - we can make it only configurable in v2 (which is easier, just define the struct with mapstructure tags)

Comment on lines +199 to +200
builder := &MappingBuilder{}
schema, _, err := builder.GetSpanServiceMappings()
Copy link
Member

Choose a reason for hiding this comment

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

where does the term "mapping" come from? If you're copying it from Elasticsearch, there "mapping" is actually a term understood by ES. No such thing in Cassandra.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should i call it template or cassandra_template ?

Copy link
Member

Choose a reason for hiding this comment

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

SchemaBuilder

case "4":
template = "./v004.cql.tmpl"
default:
template = "./v004.cql.tmpl"
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure how you are planning to use those files. They use variable placeholders like ${keyspace}, which works in shell substitution, but will not work as a Go template. We should be using Go template the this functionality. Your could do a search/replace, something like ${keyspace} ==> {{ .Keyspace }}. But it would be easier just to copy and change the syntax. You might need to use conditional clauses, like {{- if .UseILM}} from ES templates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this cqlOutput provides the same exact output as create.sh it is done in the RenderCQLTemplate function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants