-
-
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 experimental support for Amazon Managed KeySpace #2644
Conversation
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.
LGTM. Thank you @li-boxuan for this feature!
janusgraph-cql/src/main/java/org/janusgraph/diskstorage/cql/CQLKeyColumnValueStore.java
Outdated
Show resolved
Hide resolved
Closes JanusGraph#2370 Signed-off-by: Boxuan Li <liboxuan@connect.hku.hk>
d6e6880
to
6921e2a
Compare
@@ -231,6 +231,8 @@ public CQLStoreManager(final Configuration configuration) throws BackendExceptio | |||
"server, please check %s and %s options", PARTITIONER_NAME.getName(), METADATA_TOKEN_MAP_ENABLED.getName())); | |||
} | |||
switch (partitioner) { | |||
case "DefaultPartitioner": // Amazon managed KeySpace uses com.amazonaws.cassandra.DefaultPartitioner | |||
fb.timestamps(false).cellTTL(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.
I have no idea how we could possibly test this. If anyone has an idea, please let me know.
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.
I guess in this situation it should be tested manually because we don't have access to Amazon managed cassandra to test it. Also, I didn't find Docker containers which mimic Amazon managed Cassandra.
We could also refactor the code to follow DI principle and move cql session creation and store features creation to separate builders / factories. If so, we can mock session and cover store features creation by unit tests but I wouldn't do it in the same PR.
Thus, I think that's fine to not cover this branch with tests in this PR
Will merge this in 24 hours if there is no other review. |
Closes #2370
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: