Update to TinkerPop 3.2.3 #78

sjudeng · 2017-02-03T18:15:30Z

This PR includes commits by @dylanht and myself. All default and TinkerPop tests are passing (mvn clean install -Dtest.skip.tp=false). Several tests do require apache/tinkerpop#458 but in my opinion this fix is not critical to a release (e.g. it's likely just relevant to tests).

Update from 3.1.1-incubating to 3.2.0-incubating

The following is an excerpt of @dylanht's notes from his original PR (May 2016):

Missing from this PR are many practical things such as a CassandraOutputFormat, CassandraInput/OutputRDD, GraphFilter support for CassandraInputFormat, and updates to examples / docs that would really allow people to take advantage of the new features and understand how they can work with JanusGraph. Also missing from this PR is compiling g.V(x) and g.V().hasId(x) to graph.vertices(x) in JanusGraphStepStrategy for JanusGraph as I believe @okram has previously recommended. I attempted to modify the HasStepFolder.foldInHasContainers() method which seemed to match the code example in the TinkerPop upgrade docs about the new GraphStep.processHasContainerIds() helper method but couldn't get it to go.

I commented out occurrences of JanusGraphTest.verifyMetrics() because I was getting a null pointer from trying to access the TraversalMetrics stored via ...profile("metrics") and accessed via t.getSideEffects().get("metrics") after changing the test traversals to match the new syntax implemented the profiling overhaul @rjbriody did. I am hopeful I just missed what was really happening under the hood and changes to TP3ProfileWrapper will be sufficient down the line - I failed to grasp how QueryProfiler works to let JanusGraph interface with profile() step, and surprisingly in the console on an inmemory JanusGraph the same traversals seemed to work and I could access the metrics and nested metrics from getSideEffects().get("the-metrics-key-or-~metrics-whichever") as the tests attempt to do. I tried other ways of getting the metrics out of the traversal in the tests and couldn't get it done.

Update from 3.2.0-incubating to 3.2.3

The following notes are from the update PR (November 2016):

Explicitly attaching ReferenceElements before persisting in FulgoraGraphComputer to resolve numerous tests failures associated with missing properties
Opting out of GraphComputerTest#shouldSupportGraphFilter since FulgoraGraphComputer doesn't support graph filters but also can't throw the expected exception without breaking a number of tests that otherwise pass
TraverserSet and HashMap can now be serialized, but this uses Java serialization so performance will be very poor
JanusGraph assigned vertex ids can vary significantly between runs and ids for subsequently added vertices can often be smaller than those for existing vertices. This caused sporadic issues in two tests as described below. I think these issues should be addressed on the TinkerPop side but doing so will require more work to come up with appropriate test cases (not to mention solutions). In the meantime these issues were resolved by adding appropriate configuration to test JanusGraph GraphComputerProvider implementations (e.g. InMemoryGraphComputerProvider, etc.) to ensure vertex ids are evenly spaced and increasing in tests.
- PeerPressureTest#g_V_peerPressure_byXclusterX_byXoutEXknowsXX_pageRankX1X_byXrankX_byXoutEXknowsXX_timesX2X_group_byXclusterX_byXrank_sumX - The vertex program appears to cluster by vertex id. I don't think this will work (consistently) for JanusGraph unless clustering by some other property.
- UnionTest#g_VX1_2X_localXunionXoutE_count__inE_count__outE_weight_sumXX - When the assigned id of the first vertex in the test, marko, is less than that of the second vertex, vadas, then the test passes. Otherwise the test fails. In this case the local traversal is executed twice on the second (smaller id) vertex. The issue can be resolved by cloning the local step in WorkerExecutor if the associated local traversal starts with a UnionStep ... but that's just a hack and can't be recommended to TinkerPop (especially not without a test case).

References:

thinkaurelius/titan#1312
dylanht/titan#1
#41

janusgraph-bot · 2017-02-03T18:16:03Z

Committer of one or more commits is not listed as a CLA signer, either individual or as a member of an organization.

mbrukman · 2017-02-03T18:25:09Z

@dylanht – your commits list your name as "dylanht" rather than using your full name, so the CLA checker did not like that, so please fix that (I'd prefer to avoid special-casing multiple names for commits).

That said, due to a requirement from The Linux Foundation, all commits need to include the Signed-off-by: name <email> line to accept the Developer Certificate of Origin (DCO), which I don't think any of the commits here do. The CLA checker does not currently verify this.

Since you're going to add the DCO sign-off, please also update your name in the commits, and everything will be accepted by the CLA checker.

@sjudeng — the DCO similarly applies to your commits as well, but your name / email are correct as-written and as-recorded in the CLA signers list, so you just to add the sign-off.

Thanks for your help and sorry for the inconvenience.

sjudeng · 2017-02-03T18:27:18Z

Not sure why @janusgraph-bot is unhappy. Both authors are in CLA_SIGNERS. But all commits are pre-JanusGraph. For the record unless I'm mistaken I think it would be a lot of work (e.g. rebase/conflict nightmare) to go back and update names/signatures on all those old commits. Hopefully this isn't necessary.

sjudeng · 2017-02-03T18:29:26Z

@mbrukman Sorry I didn't see your response before writing mine. Commits after JanusGraph are signed but the old commits are not. I think it would be a lot of work to go back and sign those old commits. Can we pursue an exception or handle in some other way?

mbrukman · 2017-02-03T19:40:37Z

@sjudeng and @dylanht — you're all set.

sjudeng · 2017-02-03T19:55:56Z

@mbrukman Excellent, thanks!

amcp

some comments

amcp · 2017-02-04T05:12:05Z

janusgraph-core/src/main/java/org/janusgraph/core/JanusGraph.java

+@Graph.OptOut(
+        test = "org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest",
+        method = "shouldSupportGraphFilter",
+        reason = "Titan currently does not support graph filters but does not throw proper exception because doing so breaks numerous tests in gremlin-test ProcessComputerSuite.")


we should create an issue on TP to handle this exception gracefully (or provide an avenue to avoid it).

So the more correct statement here would be "graph filter support in JanusGraph is untested." FulgoraGraphComputer doesn't support graph filters, which is why the OptOut is needed. Note though I've recently used graph filters without issue on JanusGraph using SparkGraphCompter, for example.

If we were going to file an issue it would be that gremlin-test does not fully support computers that don't support graph filters. But I kind of think it's on JanusGraph ... update FulgoraGraphComputer to support graph filters (which I apologize I didn't do as part of this effort) or find a way to get rid of it (see discussion on this below).

#41
https://groups.google.com/forum/#!topic/janusgraph-dev/rmdVpvIJOXc

amcp · 2017-02-04T05:12:44Z

janusgraph-core/src/main/java/org/janusgraph/diskstorage/BackendTransaction.java

+            return BackendOperation.execute(exe, maxReadTime);
+        } catch (JanusGraphException e) {
+            // support traversal interruption
+            // TODO: Refactor to allow direct propagation of underlying interrupt exception


create an issue github so we can backlog the refactor

amcp · 2017-02-04T05:14:42Z

...rc/main/java/org/janusgraph/graphdb/database/serialize/attribute/SerializableSerializer.java

@@ -0,0 +1,35 @@
+package org.janusgraph.graphdb.database.serialize.attribute;


missing license header

amcp · 2017-02-04T05:14:50Z

...rc/main/java/org/janusgraph/graphdb/database/serialize/attribute/SerializableSerializer.java

+import java.io.Serializable;
+import java.util.HashMap;
+
+public class SerializableSerializer<T extends Serializable> implements AttributeSerializer<T>, SerializerInjected {


need classdoc

amcp · 2017-02-04T05:15:34Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/olap/computer/FulgoraGraphComputer.java

@@ -1,22 +1,7 @@
-// Copyright 2017 JanusGraph Authors


why remove the license header?

amcp · 2017-02-04T05:21:16Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/olap/computer/VertexMemoryHandler.java

        }
-        //..but only if specifically asked for by key
-        List<VertexProperty<V>> result = new ArrayList<>(Math.min(keys.length,vertexMemory.elementKeyMap.size()));
+        List<VertexProperty<V>> result = new ArrayList<>(Math.min(keys.length,memoryKeys.size()));
        for (String key : keys) {


stream.filter.forEach?

amcp · 2017-02-04T05:22:26Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/tinkerpop/optimize/JanusGraphStep.java

@@ -100,7 +100,7 @@ public void setMetrics(MutableMetrics metrics) {

    @Override
    public void addHasContainer(final HasContainer hasContainer) {
-        this.addAll(Collections.singleton(hasContainer));
+	this.addAll(Collections.singleton(hasContainer));


amcp · 2017-02-04T05:22:45Z

janusgraph-hadoop-parent/janusgraph-hadoop-2/pom.xml

@@ -50,7 +50,7 @@
        <dependency>
            <groupId>org.apache.hbase</groupId>
            <artifactId>hbase-server</artifactId>
-            <version>${hbase098.version}</version>
+            <version>${hbase098.version}</version>	    	    


remove extra spaces

amcp · 2017-02-04T05:22:52Z

janusgraph-hadoop-parent/janusgraph-hadoop/pom.xml

@@ -19,7 +19,7 @@
        <dependency>
            <groupId>${project.groupId}</groupId>
            <artifactId>janusgraph-hadoop-core</artifactId>
-            <version>${project.version}</version>
+            <version>${project.version}</version>	    	    


remove extra spaces

amcp · 2017-02-04T05:24:37Z

janusgraph-test/src/main/java/org/janusgraph/blueprints/AbstractJanusGraphProvider.java

+                try {
+                    g.close();
+                } catch (IOException | IllegalStateException e) {
+                    logger.warn("Titan graph may not have closed cleanly", e);


as part of close(), is tx().commit() also called? im poking at the log level and wonder if this should be an error

i guess this is test code. maybe we should fail() the test if we get in here...

I don't think so, just tx.close() (see StandardJanusGraph.closeInternal()). This was needed to resolve some sporadic test failures caused by uncaught exceptions associated with "transaction already closed". I can look at it again because I agree it would be better not to have to do this but note this class is in janusgraph-test (e.g. it's only used in unit tests).

Please create an issue and move on. I do not want to block 3.2.3 support at this point.

Understood thanks. I tried taking the try-catch out but I couldn't reproduce the issue in janusgraph-test or berkeleydb. So I would have had to run through the TinkerPop test suite in the cassandra and hbase modules to investigate further and each of those take 24 hours to run.

jerryjch · 2017-02-05T01:20:47Z

My question again is: Do we need to keep all the 19 commits going into JanusGraph commit history?
@sjudeng has given a nice history and description in the top description of this PR.

There are many intermediate commits and merges that are actually for Titan code. There is really no need to list them all into JanusGraph. With all due respect to the good work done by @dylanht @sjudeng, can we simplify and clean up the commits?

amcp · 2017-02-05T01:34:16Z

Some of the commits could be squashed, but preserving the history of interaction is also important for visibility and attribution purposes. Its not fair to @dylanht if all of his commits are squashed and he does not get credit in the heat / activity-map calendar on his profile as a contributor.

amcp · 2017-02-05T01:36:20Z

For example,
836b607 - 914eebf
could be squashed into one commit and @dylanht would still get credit. Given that the size of this PR is not enormous, I think it is OK to make adjustments like this.

jerryjch · 2017-02-05T02:25:22Z

@amcp I am with you on the due credit.
In a project, let's try to avoid having too many commits and merges in one PR. If we can squash them, let's do it.

we try to avoid 'work-in-progress' in formal commits.
we try to avoid commits that belong to one PR or Issue being far apart in the commit history.

jerryjch · 2017-02-05T02:36:27Z

I know we don't have a developer document or policy yet. There is a discussion on the project dev list.
Let's continue the discussion there.

jerryjch · 2017-02-07T03:37:06Z

@sjudeng
Don't want to hinder your merge here.
Here is an option or idea. See if you like it or not.
We can squash the commits into one. In the commit message, list @dylanht and you.
Similar to here:
apache/hbase@ae21797

hsaputra · 2017-02-07T04:26:25Z

You can always do git rebase master -i to pick and choose commits into manageable numbers

hsaputra · 2017-02-07T04:33:39Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/olap/computer/FulgoraMemory.java

    public Map<String, Object> previousMap;
    public Map<String, Object> currentMap;
    private final AtomicInteger iteration = new AtomicInteger(0);
    private final AtomicLong runtime = new AtomicLong(0l);
+    private boolean inExecute = false;


Given that this could be called from different threads, should add volatile modifier to make sure changes immediately visible.

Done thanks

Thx @sjudeng but looks like you may have updated the wrong variable?
The VertexMemoryHandler.inExecute does not need to have volatile modifier.

Ah sorry, should be good now. The real loser here is Travis CI, which now has an extra hour of building to do ...

sjudeng · 2017-02-07T04:36:08Z

@jerryjch We're on the same page regarding striving for a single commit per feature. On the other hand though, I don't think it would ever be appropriate to squash out another developers commits and this isn't something I'd be willing to do. I'm not sure about @dylanht but I know for my part the size of this contribution does not do justice to the amount of time it took to come up with it.

I agree with @amcp that the best solution if necessary would be for @dylanht to squash commits 836b607-914eebf. I can then combine the original update (4aca6d6) and gremlin server fix (7159656) as a single commit. I'd still keep the code cleanup (a04e44c) as a separate commit as I think that's useful to separate. I also don't think I'll be able to get away from the one additional post-JanusGraph merge commit (deb99a9).

mbrukman · 2017-02-07T05:13:55Z

janusgraph-core/src/main/java/org/janusgraph/core/JanusGraph.java

+@Graph.OptOut(
+        test = "org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest",
+        method = "shouldSupportGraphFilter",
+        reason = "JanusGraph test graph computer (FulgoraGraphComputer} " +


Done thanks

mbrukman · 2017-02-07T17:48:06Z

@jerryjch, @sjudeng – I agree on having a single, atomic commit on master for a feature, but in the case of a merge commit, does it really matter that there were N commits on the branch prior to merge? From the perspective of the master branch, all of those commits are either present or not, at any point in history.

@jerryjch and @dylanht – what's your take on @sjudeng's most recent comment?

I'd like to unblock this PR and get it merged soon.

sjudeng · 2017-02-07T18:06:52Z

Also once reviewers are satisfied I would like to run through the test suite one more time unless someone else is going to do it. This takes about 40 hours to complete so I'd like to wait until I have the all clear to run.

jerryjch · 2017-02-08T00:36:26Z

I am ok with whatever approach @sjudeng and @dylanht come up with. We all agree on striving for a clean commit history. But these are legacy commits, based on timestamps.

amcp · 2017-02-08T00:54:43Z

In the interest of progress, lets timebox this effort. If @dylanht can find an opportunity in the next day to squash his commits, then let him work with @sjudeng on that (no pressure!). Otherwise, if @dylanht does not have time, it is OK for @sjudeng to go ahead and merge the commits as is, after the tests run. We should not impede progress due to the form of legacy commits.

@rjbriody

FulgoraGraphComputer was the primary barrier to this dependency upgrade. Between 3.1.x and 3.2.x TinkerPop made significant changes to how TinkerPop-enabled GraphComputers are implemented - in particular, FulgoraMemory, FulgoraVertexMemory, and FulgoraGraphComputer classes were updated to use the new VertexComputeKey and MemoryComputeKey models in TinkerPop. Most instructive in this effort was TinkerGraphComputer and related classes git history. In order to allow MapReduce to set MemoryComputeKeys, I altered the timing at which memory.completeSubRound() is called in FulgoraGraphComputer so that this.execute would no longer be true when MapReducers were trying to add their keys to memory. I made no effort to ensure the new transient/broadcast flags are respected, and added "this" in many places to copy the TinkerGraphComputer style explicitly. The relationship of Titan's ScanJob to TinkerGraphComputerView is still opaque to me, and many comments reflecting other doubts I had about divergent implementation details between FulgoraGraphComputer and TinkerGraphComputer are found throughout. I reflected advice in the TinkerPop 3.2.x upgrade guide re: changes to ComparatorHolder, e.g. OrderXXXStep and Traveral.Admin in signatures. However, I did not manage to update HasStepFolder.foldInHasContainers in TitanGraphStepStrategy as it was updated in TinkerGraphStepStrategy for TinkerPop 3.2.0-incubating, although it looked like a drop in. FulgoraVertexMemory.getIdMap now streams vertexProgram.getVertexComputeKeys() into a HashSet, and I added a check on features().getMaxWorkers of FulgoraGraphComputer. Also fixed incorrect class name in doc comment inside ScanJob class. TitanGraphTest had many traversals featuring a LocalStep where Titan previously expected to have a TitanVertexStep, and I changed those tests to expect LocalStep where it occurs. Also in tests accessing the "~metrics" sideEffect key that based on work by @rjbriody on profiling in TinkerPop and some tests in the console should have returned TraversalMetrics was giving me a null pointer, so I commented out calls to verifyMetrics() in TitanGraphTest. I considered the logic in QueryProfiler or TP3ProfileWrapper, HasStepFolder.foldInOrder and/or HasStepFolder.foldInHasContainer, the difference in LocalStep/TitanVertexStep expectation I saw elsewhere in the tests, and GraphStep.processHasContainerIds() which I failed to update to reflect the TinkerPop 3.2.x upgrade guide as likely candidates for this issue. Hopefully TP3ProfileWrapper is all we need to consider. TitanH1OutputFormat was changed and I am worried that it needs to respect isTransient() for persistableKeys. I updated the poms as needed, and @sjudeng figured out my initial confusion around curator recipes, which only needed to be included at the right version in the top-level pom.xml file. Signed-off-by: dylanht <[email protected]>

dylanht · 2017-02-08T04:35:15Z

@amcp @mbrukman @jerryjch @sjudeng agree completely with the assessment here that my commits should be squashed despite the fact that it probably isn't the most consequential case given timing and greater importance of getting this PR through. Sorry to be so late to join in here we're in the middle of moving out/in.

@sjudeng I squashed the commits on my branch at https://github.com/dylanht/titan/tree/upgrade-to-tinkerpop-3.2.0-incubating, and invited you as a collaborator in case that proves useful. I saw your merge commit here (ngageoint@d4b4618) - I would think you're planning to pull the squashed commits down and force-push to your branch on ngageoint but if you had something else in mind let me know if I can help!

twilmes · 2017-02-08T04:31:12Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/tinkerpop/JanusGraphFeatures.java

@@ -78,7 +78,7 @@ public static JanusGraphFeatures getFeatures(StandardJanusGraph graph, StoreFeat

        @Override
        public boolean supportsMapValues() {
-            return false;
+            return true;


This might be unrelated, but is this a reversion back to the pre-1.0 support for vertex/edge property values of type map? If I remember correctly, this was pulled out to prevent having to deal with serialization of any old type inserted as a map value. I think this may become even more important with Gremlin language variants because we'll likely have a greater number of non-JVM based languages using JanusGraph. If we do want to support map values, perhaps we should add extra validation in to ensure that the maps values fall into some set of supported types. I haven't looked at the serialization code close enough, but perhaps the newer serialization will at least enforce the types of keys and values.

Regardless, If we're going to contemplate adding map back in, I think that should be pulled out into a separate discussion.

This update was to resolve test failures and involved adding an initial implementation for HashMap serialization. The implementation simply uses Java object serialization, which will throw a java.io.NotSerializableException if the map contains unsupported types.

As noted in the PR notes there are definitely at least performance issues with using Java serialization here. Can we create a separate issue to update the initial HashMap serialization implementation to both improve performance and formalize validation?

Can you speak a bit more about the test failures that required the change? Are there TinkerPop tests that were requiring map support even with the feature disabled? Regardless, I'm ok with moving forward on this to get it merged but I'll start a discussion on the list so that we can reach consensus on whether or not map support should be added back in and if it is, what the extra validation implications are. At that point, if we move forward with it, we can create an issue.

Yes, for example in org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest the shouldSucceedWithProperTraverserRequirements and shouldFailWithImproperTraverserRequirements tests throw IllegalArgumentExceptions (Property value [{...}] is of type class java.util.HashMap is not supported) with supportsMapValues=false and without the associated serialization implementation. See attached stack trace for details.

twilmes · 2017-02-08T04:31:41Z

janusgraph-test/src/main/java/org/janusgraph/graphdb/JanusGraphTest.java

-        verifyMetrics(metrics.getMetrics(0), true, false);
-        verifyMetrics(metrics.getMetrics(1), true, true);
+        metrics = (TraversalMetrics) t.asAdmin().getSideEffects().get("~metrics");
+        //verifyMetrics(metrics.getMetrics(0), true, false);


Can this commented out code be removed?

twilmes

Thanks for all the hard work @sjudeng and @dylanht

VOTE: +1

…significant updates to FulgoraGraph Computer and associated memory implementation, support for GraphSON 2.0 and support for interrupts in HBase backend. Update Gremlin server configuration to remove reference to pure nashorn ScriptEngine, which is no longer supported. Opt out of IoTest#shouldReadGraphMLWithNoEdgeLabel and GraphComputerTest#shouldSupportGraphFilter (see reasons in OptOut declarations). Skip titan-hadoop-1 tests (hadoop1 is no longer supported). Signed-off-by: sjudeng <[email protected]>

…inst titan11 Signed-off-by: sjudeng <[email protected]>

…ntation Signed-off-by: sjudeng <[email protected]>

sjudeng · 2017-02-09T03:36:41Z

Thanks @dylanht. As mentioned above I also combined two of my pre-JanusGraph commits but did keep code formatting commit separate and I also needed one post-merge commit.

sjudeng · 2017-02-09T23:59:12Z

All tests (mvn clean install -Dtest.skip.tp=false) ran with no issues. Note that these were run on a branch that included the unsquashed commits in this branch as well as the commits in #79. Also janusgraph-hbase-10 tests were run but not janusgraph-hbase-098 to save time.

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 40:28:24.762s
[INFO] Finished at: Thu Feb 09 20:20:18 UTC 2017
[INFO] Final Memory: 137M/1557M
[INFO] ------------------------------------------------------------------------

mbrukman · 2017-02-13T02:24:10Z

I think we've had quite a long discussion on this PR, and I believe all concerns have been addressed, all the tests are passing, so we should move forward on this PR. In particular, @hsaputra, @jerryjch, and @pluradj — any other comments? If you're OK with this PR as it stands now, please feel free to approve.

I am proposing we have a 24-hour comment period, and unless any objections are brought up during this time period, either I or @amcp will merge the PR.

hsaputra · 2017-02-13T06:07:36Z

+1 agree

Let's proceed. The number of commits are manageable now so LGTM

amcp · 2017-02-14T02:26:56Z

a day has passed. merging this, and we can open new issues/prs if there are any questions.

amcp · 2017-02-22T04:40:27Z

@sjudeng Do you have the test output saved? otherwise, ill just run it again.

sjudeng · 2017-02-22T15:12:39Z

@amcp Yes, that's what I used the generate the table in #111. I'll grep out the relevant lines (e.g. module and individual test suite times) and post later today.

amcp · 2017-02-22T15:32:00Z

@sjudeng I may be seeing ghosts but on one of my feature branches i am not able to complete mvn clean install -Dtest.skip.tp=false. I need to dig deeper. Anyway, any information you can share from the run is greatly appreciated.

sjudeng · 2017-02-23T01:05:57Z

@amcp Here you go: test_all_tp32_excerpt.txt

Note that output includes individual test suites as well as a total line for each module, which you want to include/exclude depending on what you're looking at.

In case useful here's the Python script I used to parse and create table in #111.

import os,re,sys

s = os.popen('grep -e Building -e ^Tests %s | grep -v jar' % sys.argv[1]).read()

modules = re.findall('(Building.*?)\[INFO\]',s,re.DOTALL|re.M)

print('| Module | Test Count | Time (sec) |')
print('| ------ | ---------- | ---------- |')
times = []
counts = []
for module in [module for module in modules if 'Time elapsed' in module]:
    name = module.split('\n')[0].split()[1]
    u = dict([(i,0) for i in re.findall('Time elapsed: .*? (.*?) ',module)])
    assert(len(u)==1)
    total_sec = sum(map(float,[i.replace(',','') for i in re.findall('Time elapsed: (.*?) ',module)]))
    count_block = '\n'.join([line for line in module.split('\n') if 'Time' not in line])
    total_count = sum(map(int,re.findall('Tests run: (\d+)',count_block)))
    times.append(total_sec)
    counts.append(total_count)
    print('| %s | %s | %.0f |' % (name,total_count,total_sec))

print('| Total | %.0f | %.0f |' % (sum(counts),sum(times)))

amcp · 2017-02-23T12:22:21Z

This is great. I'll put together a plan in Excel. What is the meaning of line 179 of your attachment? looks like a subtotal but it is not prefaced by anything.

sjudeng · 2017-02-23T13:47:46Z

That's the total line for the default tests in janusgraph-test. Then L180-L487 are the results for the TinkerPop tests in janusgraph-test and L488 is the corresponding total. Sorry for the confusion, I didn't include the test execution name in the excerpt. I'll update the file later today.

Update to TinkerPop 3.2.3

janusgraph-bot added the cla: no This PR is not compliant with the CLA label Feb 3, 2017

janusgraph-bot added cla: yes This PR is compliant with the CLA and removed cla: no This PR is not compliant with the CLA labels Feb 3, 2017

mbrukman requested review from hsaputra, jerryjch, twilmes and pluradj February 3, 2017 19:40

amcp reviewed Feb 4, 2017

View reviewed changes

sjudeng mentioned this pull request Feb 4, 2017

Issue 67: Update Elasticsearch and improve geoshape indexing #79

Merged

amcp approved these changes Feb 5, 2017

View reviewed changes

sjudeng mentioned this pull request Feb 5, 2017

Improve support for traversal interrupts #88

Open

sjudeng force-pushed the tinkerpop-upgrade branch from 87861c1 to deb99a9 Compare February 7, 2017 02:04

hsaputra reviewed Feb 7, 2017

View reviewed changes

sjudeng force-pushed the tinkerpop-upgrade branch 2 times, most recently from d9fd577 to 5e12bf5 Compare February 7, 2017 05:08

mbrukman reviewed Feb 7, 2017

View reviewed changes

twilmes reviewed Feb 8, 2017

View reviewed changes

twilmes approved these changes Feb 8, 2017

View reviewed changes

sjudeng added 4 commits February 8, 2017 19:10

Formatting updates to use spaces for indentation and cleanup diff aga…

a7bb8e7

…inst titan11 Signed-off-by: sjudeng <[email protected]>

Merge remote-tracking branch 'origin/master' into tinkerpop-upgrade

a4ace7b

Code cleanup, add missing headers, fix legacy names and update docume…

c2431d5

…ntation Signed-off-by: sjudeng <[email protected]>

sjudeng force-pushed the tinkerpop-upgrade branch from 5665f8c to c2431d5 Compare February 9, 2017 03:29

jerryjch approved these changes Feb 13, 2017

View reviewed changes

amcp merged commit e6e0067 into JanusGraph:master Feb 14, 2017

sjudeng deleted the tinkerpop-upgrade branch February 16, 2017 01:59

sjudeng mentioned this pull request Mar 3, 2017

Enable setting custom vertex ids in TP3 interface #147 #148

Merged

sjudeng mentioned this pull request Apr 21, 2017

CQL storage backend #166

Merged

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

Merge pull request JanusGraph#78 from ngageoint/tinkerpop-upgrade

486f4ae

Update to TinkerPop 3.2.3

		@@ -0,0 +1,35 @@
		package org.janusgraph.graphdb.database.serialize.attribute;

Update to TinkerPop 3.2.3 #78

Update to TinkerPop 3.2.3 #78

Conversation

sjudeng commented Feb 3, 2017

Update from 3.1.1-incubating to 3.2.0-incubating

Update from 3.2.0-incubating to 3.2.3

References:

janusgraph-bot commented Feb 3, 2017

mbrukman commented Feb 3, 2017

sjudeng commented Feb 3, 2017

sjudeng commented Feb 3, 2017

mbrukman commented Feb 3, 2017

sjudeng commented Feb 3, 2017

amcp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryjch commented Feb 5, 2017 • edited Loading

amcp commented Feb 5, 2017

amcp commented Feb 5, 2017

jerryjch commented Feb 5, 2017

jerryjch commented Feb 5, 2017

jerryjch commented Feb 7, 2017

hsaputra commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjudeng commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrukman commented Feb 7, 2017

sjudeng commented Feb 7, 2017

jerryjch commented Feb 8, 2017

amcp commented Feb 8, 2017

dylanht commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twilmes left a comment

Choose a reason for hiding this comment

sjudeng commented Feb 9, 2017

sjudeng commented Feb 9, 2017 • edited Loading

mbrukman commented Feb 13, 2017 • edited Loading

hsaputra commented Feb 13, 2017

amcp commented Feb 14, 2017

amcp commented Feb 22, 2017

sjudeng commented Feb 22, 2017

amcp commented Feb 22, 2017

sjudeng commented Feb 23, 2017 • edited Loading

amcp commented Feb 23, 2017

sjudeng commented Feb 23, 2017

jerryjch commented Feb 5, 2017 •

edited

Loading

sjudeng commented Feb 9, 2017 •

edited

Loading

mbrukman commented Feb 13, 2017 •

edited

Loading

sjudeng commented Feb 23, 2017 •

edited

Loading