Skip to content

Conversation

@mahadzaryab1
Copy link
Collaborator

@mahadzaryab1 mahadzaryab1 commented Sep 6, 2024

Which problem is this PR solving?

Description of the changes

  • Revamped configuration for Cassandra to be logically grouped
  • Added unit tests for the Validate method
  • Changed the configuration to use OTEL's configtls.ClientConfig type
  • Migration guide

How was this change tested?

  • New unit tests
  • CI

Checklist

@codecov
Copy link

codecov bot commented Sep 6, 2024

Codecov Report

Attention: Patch coverage is 97.95918% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.90%. Comparing base (d7f01d1) to head (2f1f0e9).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/cassandra/config/config.go 96.72% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5949      +/-   ##
==========================================
+ Coverage   96.80%   96.90%   +0.10%     
==========================================
  Files         349      349              
  Lines       16581    16582       +1     
==========================================
+ Hits        16051    16069      +18     
+ Misses        342      329      -13     
+ Partials      188      184       -4     
Flag Coverage Δ
badger_v1 8.00% <0.00%> (-0.01%) ⬇️
badger_v2 1.82% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v1 15.78% <87.75%> (-0.79%) ⬇️
cassandra-4.x-v2 1.75% <0.00%> (-0.01%) ⬇️
cassandra-5.x-v1 15.78% <87.75%> (-0.79%) ⬇️
cassandra-5.x-v2 1.75% <0.00%> (-0.01%) ⬇️
elasticsearch-6.x-v1 18.72% <0.00%> (-0.01%) ⬇️
elasticsearch-7.x-v1 18.78% <0.00%> (-0.01%) ⬇️
elasticsearch-8.x-v1 18.98% <0.00%> (-0.01%) ⬇️
elasticsearch-8.x-v2 1.82% <0.00%> (-0.01%) ⬇️
grpc_v1 9.53% <0.00%> (-0.01%) ⬇️
grpc_v2 7.12% <0.00%> (-0.02%) ⬇️
kafka-v1 9.71% <0.00%> (-0.01%) ⬇️
kafka-v2 1.82% <0.00%> (-0.01%) ⬇️
memory_v2 1.82% <0.00%> (-0.01%) ⬇️
opensearch-1.x-v1 18.84% <0.00%> (-0.01%) ⬇️
opensearch-2.x-v1 18.84% <0.00%> (-0.01%) ⬇️
opensearch-2.x-v2 1.81% <0.00%> (-0.02%) ⬇️
tailsampling-processor 0.46% <0.00%> (-0.01%) ⬇️
unittests 95.70% <95.91%> (+0.41%) ⬆️

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.

@mahadzaryab1 mahadzaryab1 marked this pull request as ready for review September 12, 2024 01:44
@mahadzaryab1 mahadzaryab1 requested a review from a team as a code owner September 12, 2024 01:44
Signed-off-by: Mahad Zaryab <[email protected]>
@mahadzaryab1
Copy link
Collaborator Author

Is NamespaceConfig still relevant (https://github.com/jaegertracing/jaeger/blob/main/plugin/storage/cassandra/options.go#L70-L74)? Or can we replace it with the Configuration type?

@mahadzaryab1
Copy link
Collaborator Author

@yurishkuro This is ready for another look. The patch code coverage is failing because of a couple of lines which were modified that I'm assuming weren't tested before. Should I add some tests for NewCluster?

@yurishkuro yurishkuro added the changelog:exprimental Change to an experimental part of the code label Sep 25, 2024
Signed-off-by: Yuri Shkuro <[email protected]>
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Thanks!

@yurishkuro yurishkuro enabled auto-merge (squash) September 26, 2024 01:18
@yurishkuro
Copy link
Member

migration doc not readable

yurishkuro and others added 2 commits September 25, 2024 21:22
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
auto-merge was automatically disabled September 26, 2024 04:00

Head branch was pushed to by a user without write access

@mahadzaryab1
Copy link
Collaborator Author

@yurishkuro thank you so much for writing the tests! the linter was failing so I just fixed that. Auto merge was disabled so will need to be enabled or merged in again.

@mahadzaryab1
Copy link
Collaborator Author

migration doc not readable

sorry about that! just opened the access up

@mahadzaryab1
Copy link
Collaborator Author

I think we have a one-off failure for the ES test. I think a rerun of the CI should fix it since we didn't touch anything ES related in this PR.

@yurishkuro yurishkuro merged commit 3cd1f59 into jaegertracing:main Sep 26, 2024
49 checks passed
@mahadzaryab1 mahadzaryab1 deleted the cassandra-storage-v2 branch September 26, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/storage changelog:exprimental Change to an experimental part of the code storage/cassandra v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[jaeger-v2] Align Cassandra Storage Config With OTEL

2 participants