-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 ScyllaDB driver support #3578
Add ScyllaDB driver support #3578
Conversation
5f72e86
to
bd75102
Compare
9e3a4c2
to
e650702
Compare
@porunov Would it be possible to use java shadowing for instead of library coping? |
I don't have experience with shadowing Java libraries, so can't say for sure. I have created an issue #3580 to be able to avoid copying libraries. In that issue I proposed making diff between |
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.
Thanks a lot for tackling this, @porunov! I also tried that a while back, but then stopped when I ran into the issue you're describing in #3580 as I had no idea how to resolve this. But your approach of first only adding this as a module and resolve the problem with selecting the right library in the distribution at runtime in a follow-up issue definitely makes sense to get us moving here :-)
I just reviewed the diff here and everything looks good basically. I only added some small nitpick comments. The logs of the added GH Actions also show that the Scylla driver is really being used there:
Using Scylla optimized driver!!!
I would like to also build the distribution and try out the approach you've documented with manually replacing the JARs if I find some time in the next few days but wanted to already provide my review comments so far.
janusgraph-scylla/src/main/java/org/janusgraph/diskstorage/cql/ScyllaStoreManager.java
Outdated
Show resolved
Hide resolved
janusgraph-cql/src/test/java/org/janusgraph/JanusGraphCassandraContainer.java
Outdated
Show resolved
Hide resolved
Thanks @FlorianHockmann for the review! I will experiment tomorrow or on Wednesday with ScyllaDB parameters suggested here #2505 (comment) and update the PR with your suggestions and probably modify the JanusGraphCassandraContainer a bit to use those parameters. I don't personally like the manual approach of replacing libs and also think we should handle #3580 before we release 1.0.0, but thought it makes sense to ship Scylla module first. |
0329556
to
4027981
Compare
@FlorianHockmann I fixed the comments you left. I also fixed Hadoop tests. |
I'm stuck with the issue about ssl certificates (here). I'm not sure I'll be able to resolve that issue. Thus, I can just comment out tests related to ScyllaDB ssl certificates and open a new issue for the tests. Let me know what you think about it. |
That would definitely be ok for me. This PR is already a huge improvement. It doesn't need to fix everything at once and these tests would already have failed before, if they were properly configured, right? |
Yes, if they were properly configured and use that configuration they would fail due to startup issue of the testcontainer. |
- Add ScyllaDB driver support via `janusgraph-scylla` - `janusgraph-cql` contains tests for Cassandra3, Cassandra4, ScyllaDB. `janusgraph-scylla` contains tests for ScyllaDB only. - Fix ScyllaDB tests (configs were not applied to ScyllaDB tests previously) Fixes JanusGraph#1778 Related to JanusGraph#2451 Fixes JanusGraph#2505 Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
4027981
to
7179cbb
Compare
Commented the failing tests. The PR is ready for re-review |
janusgraph-scylla
janusgraph-cql
contains tests for Cassandra3, Cassandra4, ScyllaDB.janusgraph-scylla
contains tests for ScyllaDB only.ScyllaDB driver has the same classpath as DataStax driver. Thus, we can't have both
janusgraph-cql
andjanusgraph-scylla
dependencies in the classpath at the same time.It means that
janusgraph-all
can have only one of them. This is fine I think because anyone who is using that dependency should be able to excludejanusgraph-cql
and includejanusgraph-scylla
when necessary.That said, it also means that we can't have
janusgraph-scylla
in any of the JanusGraph distributions because those distributions already havejanusgraph-cql
dependency. Right now the only option to addscylla
as a storage adapter to JanusGraph distribution is by manually replacing libraries.The issue to make it more easier for users to replace
cql
drivers withscylla
drivers is here: #3580 (has a solution description).I checked that the optimized ScyllaDB driver is used for
scylla
storage option.Fixes #1778
Related to #2451
Fixes to #2505 (comment about observations of Hadoop tests with ScyllaDB is here)
Signed-off-by: Oleksandr Porunov alexandr.porunov@gmail.com
Thank you for contributing to JanusGraph!
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
master
)?For code changes:
For documentation related changes: