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

Define ScyllaDB as drop-in replacement for Cassandra #4353

Merged
merged 5 commits into from
Mar 31, 2023

Conversation

mkorolyov
Copy link
Contributor

Which problem is this PR solving?

Defines that Jaeger server supports ScyllaDB as a storage backend as a drop-in replacement for Cassandra

Short description of the changes

Updated README.md

@mkorolyov mkorolyov requested a review from a team as a code owner March 29, 2023 20:51
@mkorolyov mkorolyov requested a review from albertteoh March 29, 2023 20:51
@mkorolyov
Copy link
Contributor Author

i can provide a demo docker-compose file but have no idea where to place it to make it easily discoverable by the community

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.

We can add a folder plugins/storage/scylladb with a README and a docker-compose file. The README can call out any relevant details (or any differences vs. Cassandra). But I would like to have a clear disclaimer at the top that this is not an officially supported backend, meaning that we will not proactively address any issues that may arise from incompatibilities between the two databases (we may still accept PRs).

@yurishkuro
Copy link
Member

Please make sure all commits are signed (or squash into one and sign).

@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01 ⚠️

Comparison is base (bc8e1d1) 97.10% compared to head (bfbace7) 97.10%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4353      +/-   ##
==========================================
- Coverage   97.10%   97.10%   -0.01%     
==========================================
  Files         300      300              
  Lines       17697    17697              
==========================================
- Hits        17185    17184       -1     
- Misses        412      413       +1     
  Partials      100      100              
Flag Coverage Δ
unittests 97.10% <ø> (-0.01%) ⬇️

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

see 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mkorolyov mkorolyov requested review from yurishkuro and removed request for albertteoh March 30, 2023 17:39
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.

lgtm, but please make sure commits are signed (see CONTRIBUTING.md)


#### Protocol version

Jaeger server detects Cassandra protocol version automatically. At the date of the demo with specified versions server detects that it connected via protocol version 3 while it is actually 4. This leads to warn log in cassandra-schema container:
Copy link
Member

Choose a reason for hiding this comment

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

Sounds like maybe there should be a config param in Jaeger to set the version explicitly instead of sniffing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have tried both

      CASSANDRA_PROTOCOL_VERSION: 4
      CASSANDRA_VERSION: 4

but nothing helped. It depects the version from interaction with ScyllaDB

mkorolyov and others added 4 commits March 30, 2023 23:01
Co-authored-by: Yuri Shkuro <[email protected]>
Signed-off-by: Maxim Korolyov <[email protected]>
Signed-off-by: Maxim Korolyov <[email protected]>
Signed-off-by: Maxim Korolyov <[email protected]>
@mkorolyov mkorolyov force-pushed the define-scylladb-support branch from 769b6a4 to bfbace7 Compare March 30, 2023 21:01
@mkorolyov mkorolyov requested a review from yurishkuro March 30, 2023 21:27
yurishkuro
yurishkuro previously approved these changes Mar 31, 2023
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.

few small improvements

@mkorolyov
Copy link
Contributor Author

Hi @yurishkuro
Thanks for the review. I reworked health checks and simplified things a lot. Also, I applied all the style nits you posted above.

I still use docker compose up -d instead of the suggested docker compose up because without background mode console is filled with tons of logs from 3 ScyllaDB nodes communicating with each other to gather into a single cluster and even after that it is not obvious that you can start using HotRod or JaegerUI. While with background mode it is transparent that ScyllaDB is starting for some time and then all other containers are spinning up and you can try JaegerUI or HotRod.

@mkorolyov mkorolyov requested a review from yurishkuro March 31, 2023 16:03
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. One possible minor improvement is to pull the Jaeger and ScyllaDB versions into variables at the top of the compose file.

@yurishkuro yurishkuro merged commit ea55beb into jaegertracing:main Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants