Static cluster/session management in CQL backend #384

sjudeng · 2017-07-03T00:58:37Z

Makes the following updates to fix #383. With these updates the full (default+TinkerPop) test suite runtime for the CQL module is decreased from 15.1 hours to 3.3 hours.

A single static cluster instance and a single static session per keyspace managed in CQLStoreManager.
Truncate tables instead of drop keyspace in CQLStoreManager.clearStorage(). There was an issue with test failures when switching to truncation that was resolved by deleting the commitlog directory prior to starting embedded test instance in CassandraStorageSetup.
Test parallelism was removed to resolve test failures associated with the above updates and for consistency with test executions in other modules.
Minor updates to split Travis build matrix for CQL tests and reduce logging (also renamed logging appender names for consistency with other modules).

sjudeng · 2017-07-03T01:07:25Z

@pkendall64 Would appreciate you taking a look at this to make sure the updates here seem reasonable. One of the main performance improvements came from switching back to table truncation (which I know you had originally) in clearStorage. As mentioned above the errors that occurred when making this switch were resolved by ensuring commitlog directory was deleted. Not sure why this is required for CQL and not Thrift.

amcp · 2017-07-03T02:45:07Z

@sjudeng instead of one build target now there are four travis builds for cql. How long does each of the lines 43-46 take? let me see if i can find that in the build.

amcp · 2017-07-03T02:50:52Z

15, 10, 16 and 14 minutes. OK, great. https://travis-ci.org/JanusGraph/janusgraph/builds/249466821?utm_source=github_status&utm_medium=notification

sjudeng · 2017-07-03T02:53:09Z

@amcp Yep. The Travis jobs for CQL (like Cassandra and HBase) are split by test execution and package. Test timing in Travis can vary considerably based on time of day and other external factors. Also by keeping individual Travis job times below 15 minutes they can get the full benefit of travis_retry, which will retry up to three times but only within the overall 50 minute job timeout.

amcp

minor questions

amcp · 2017-07-03T02:51:44Z

janusgraph-cql/src/main/java/org/janusgraph/diskstorage/cql/CQLStoreManager.java

    private static final int DEFAULT_PORT = 9042;

+    private static Cluster cluster;
+    private static Map<String,Session> sessions = new ConcurrentHashMap<>();


We are not running tests in parallel anymore as per the xml file. why does this need to be concurrent? As on line 117 there is only one keyspace variable for this store manager so what else will go in the sessions map? I searched the original file and this file and I do not see any other uses of the sessions map. Finally, initializeSession is private and it seems to be the only method that mutates the session map.

@sjudeng I get it, its static. You are considering the case of multiple cql store manager instances in the same JVM. When did this come up in testing?

@amcp No it might not be a problem in testing. Do you think it might cause issues to use concurrent data structure and/or synchronized modifier here?

no problem here.

amcp · 2017-07-03T02:54:38Z

janusgraph-cql/src/main/java/org/janusgraph/diskstorage/cql/CQLStoreManager.java

+            sessions.put(keyspaceName, cluster.connect());
+            final Configuration configuration = getStorageConfig();
+            final Map<String, Object> replication = Match(configuration.get(REPLICATION_STRATEGY)).of(
+                Case($("SimpleStrategy"), strategy -> HashMap.<String, Object>of("class", strategy, "replication_factor", configuration.get(REPLICATION_FACTOR))),


What is the dollar sign for? Do we need to use it?

@amcp These are showing as changes but it's just whitespace (indented under the cluster/session==null check). This is original code from #166.

The $ turns a predicate into a pattern and most of the vavr APIs take patterns in the 0.9 version.

amcp · 2017-07-03T02:55:14Z

janusgraph-cql/src/main/java/org/janusgraph/diskstorage/cql/CQLStoreManager.java

+        final Future<Seq<ResultSet>> result = Future.sequence(
+            Iterator.ofAll(cluster.getMetadata().getKeyspace(this.keyspace).getTables())
+                                .map(table -> Future.fromJavaFuture(sessions.get(this.keyspace).executeAsync(truncate(this.keyspace, table.getName())))));
+        result.await();


no timeout for await?

Same comment as above

pkendall64 · 2017-07-03T09:28:44Z

Yeah, looks good to me. I have to wonder whether it would be better to use a wrapper to provide the caching in the test area rather than baking it into the main code directly though.

sjudeng · 2017-07-03T11:18:24Z

@pkendall64 That's not a bad idea. Probably would be a little ugly but at least it would contain it to test code. I'll take a look.

The suggestion to use the singleton cluster/session comes from http://www.datastax.com/dev/blog/4-simple-rules-when-using-the-datastax-drivers-for-cassandra. But now that I think about it if there needs to be support for connecting to different Cassandra cluster's in the same JVM this would require more updates than provided here and would probably get pretty messy.

…per to manage cluster and session statically in CQL tests. Signed-off-by: sjudeng <sjudeng@users.noreply.github.com>

amcp · 2017-07-04T17:15:32Z

@sjudeng maybe for testing it is useful to connect to be able to connect to different Cassandra clusters, but I think we can just convert the same-jvm-multiple-cluster-cql support issue into an issue on GitHub and move on with these changes?

sjudeng · 2017-07-04T21:19:23Z

@amcp I think it's moot now ... I just pushed updates that refactor to use a wrapper class during testing (thanks @pkendall64). As expected it's a little ugly (needed some reflection on StandardStoreManager) but almost all updates are in test code only. I think it makes a lot more sense this way as it allows clients to build in caching themselves if necessary (e.g. caching CQLStoreManager instances by physical cluster and/or keyspace).

amcp

looking forward to getting test time down.

Static cluster/session management in CQL backend

janusgraph-bot added the cla: yes This PR is compliant with the CLA label Jul 3, 2017

sjudeng force-pushed the cql-performance branch from 98b949f to aa2ecc8 Compare July 3, 2017 01:09

amcp self-assigned this Jul 3, 2017

amcp reviewed Jul 3, 2017

View reviewed changes

Truncate tables instead of drop kepspace in CQLStoreManager. Use wrap…

8f1387f

…per to manage cluster and session statically in CQL tests. Signed-off-by: sjudeng <sjudeng@users.noreply.github.com>

sjudeng force-pushed the cql-performance branch from aa2ecc8 to 8f1387f Compare July 4, 2017 18:55

amcp approved these changes Jul 7, 2017

View reviewed changes

sjudeng merged commit e741dce into JanusGraph:master Jul 7, 2017

sjudeng mentioned this pull request Jul 7, 2017

Enable automated testing on Travis CI #74

Closed

sjudeng mentioned this pull request Aug 13, 2017

Support graph/index deletion in core factory API #462

Closed

bwatson-rti-org pushed a commit to bwatson-rti-org/janusgraph that referenced this pull request Mar 9, 2019

Merge pull request JanusGraph#384 from sjudeng/cql-performance

7db3238

Static cluster/session management in CQL backend

micpod pushed a commit to micpod/janusgraph that referenced this pull request Nov 5, 2019

Merge pull request JanusGraph#384 from sjudeng/cql-performance

13c4e84

Static cluster/session management in CQL backend

Uh oh!

Static cluster/session management in CQL backend #384

Static cluster/session management in CQL backend #384

Uh oh!

Conversation

sjudeng commented Jul 3, 2017

Uh oh!

sjudeng commented Jul 3, 2017

Uh oh!

amcp commented Jul 3, 2017

Uh oh!

amcp commented Jul 3, 2017

Uh oh!

sjudeng commented Jul 3, 2017

Uh oh!

amcp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkendall64 commented Jul 3, 2017

Uh oh!

sjudeng commented Jul 3, 2017

Uh oh!

amcp commented Jul 4, 2017

Uh oh!

sjudeng commented Jul 4, 2017

Uh oh!

amcp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants