If i'm thinking about this right....I guess Gryo used the default Kryo serialization for 1.0 and 3.0 so by changing that we effectively break both version of Gryo in 3.4.0. And that would mean that you couldn't connect to older versions of the Java driver to 3.4.0 systems - you would have to upgrade. Is that the correct conclusion?
On Mon, Nov 12, 2018 at 9:28 AM Daniel Kuppitz <[email protected]> wrote: > I actually opened a PR <https://github.com/apache/tinkerpop/pull/993> > (makes it easier to look at the changes). Removing transient didn't quite > do the job, so I overwrote the (de)serialization methods and only kept the > parent element id. The only thing that's broken now is the serialization > test suite. > > Cheers, > Daniel > > > On Mon, Nov 12, 2018 at 4:59 AM Stephen Mallette <[email protected]> > wrote: > > > I don't think we can remove the transient keyword. Doesn't that balloon > the > > amount of data shuffled around the network? I think that was the reason > we > > made that transient in the first place. > > > > What parts of the test suite are failing because of this? Could you post > > links to a couple of examples of failing assertions? > > > > On Sat, Nov 10, 2018 at 2:01 PM Daniel Kuppitz <[email protected]> wrote: > > > > > Removing the transient attribute from the DetachedVertexProperty's > vertex > > > field [1] makes all Spark tests pass. Easy peasy, but holy cow, that > > little > > > change probably has a huge impact on things I can't even think of right > > > now. > > > > > > [1] > > > > > > > > > https://github.com/apache/tinkerpop/blob/master/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/detached/DetachedVertexProperty.java#L40 > > > > > > Cheers, > > > Daniel > > > > > > > > > On Sat, Nov 10, 2018 at 11:49 AM Daniel Kuppitz <[email protected]> > wrote: > > > > > > > Treating null parents as a wildcard makes the majority of tests just > > > pass, > > > > but Spark integration tests fail. Spark is all about detached > elements > > > (or > > > > reference elements) and I guess there's no way to make those tests > pass > > > > without making detached properties remember their parent elements (or > > at > > > > least just their parent element ids). > > > > > > > > Cheers, > > > > Daniel > > > > > > > > > > > > On Fri, Nov 9, 2018 at 4:53 AM Stephen Mallette < > [email protected]> > > > > wrote: > > > > > > > >> If we can, I think that we should just keep derser semantics as they > > > are. > > > >> We've gone this long with them having null parent element ids and > > there > > > >> haven't been any raised complains that I'm aware of. > > > >> > > > >> > I thought about skipping the parent id comparison in case one of > the > > > >> elements parents is *null* > > > >> > > > >> I suppose the question we need to answer stated directly is: What do > > we > > > >> want equals() to mean for "detached" properties? Maybe it's not > wrong > > to > > > >> consider "detached" properties without a parent to have a form of > > > wildcard > > > >> status. If you were to "attach" such a property then its id space > > would > > > be > > > >> restricted to the element you're adding it to, so it seems safe in > > that > > > >> respect. In the "tree test" that's failing that you referenced, the > > > >> deserialized tree is a wildcard match for the original tree...the > idea > > > >> sorta works in that capacity, but perhaps there are other scenarios > > > where > > > >> that's a really bad thing? > > > >> > > > >> Does a "null" parent element mean "detached"? I guess the answer is > > > >> "sometimes" because in some cases "detached" keeps the parent > element. > > > Are > > > >> there other scenarios though where the parent element can be null? I > > > can't > > > >> really think of any right now. > > > >> > > > >> On Thu, Nov 8, 2018 at 12:48 PM Daniel Kuppitz <[email protected]> > > wrote: > > > >> > > > >> > Working on TINKERPOP-2051 > > > >> > <https://issues.apache.org/jira/browse/TINKERPOP-2051>, I > thought I > > > >> found > > > >> > a > > > >> > pretty easy solution: In order to allow locally unique property > ids, > > > we > > > >> > only need to compare the ids of two properties as well as the ids > of > > > >> their > > > >> > parent elements. That was basically a one-line change and had the > > > >> expected > > > >> > effect: > > > >> > > > > >> > > > > >> > gremlin> g = TinkerGraph.open().traversal() > > > >> > ==>graphtraversalsource[tinkergraph[vertices:0 edges:0], standard] > > > >> > gremlin> g.addV("person").property("name", "alice", id, > > > "name").as("a"). > > > >> > ......1> addV("person").property("name", "bob", id, "name"). > > > >> > ......2> addE("knows").from("a") > > > >> > ==>e[2][0-knows->1] > > > >> > gremlin> > > > >> > gremlin> g.V().properties("name") > > > >> > ==>vp[name->alice] > > > >> > ==>vp[name->bob] > > > >> > gremlin> g.V().properties("name").dedup() > > > >> > ==>vp[name->alice] > > > >> > ==>vp[name->bob] > > > >> > > > > >> > > > > >> > Without the change in id comparison, the last statement would have > > > only > > > >> > returned 1 of the properties. Looked easy enough, but - of course > - > > it > > > >> > broke the test suite. This change had an impact on 2 types of > tests: > > > >> > > > > >> > - those, that deal with serialization and deserialization > > > >> > - those, that make use of bytecode > > > >> > > > > >> > Perhaps the latter can be seen as a more specific case of the > > former. > > > >> The > > > >> > problem is that serialized/deserialized/detached properties no > > longer > > > >> have > > > >> > a reference to their former parent element. I thought about > skipping > > > the > > > >> > parent id comparison in case one of the elements parents is > *null*, > > > but > > > >> at > > > >> > some point that just felt wrong and made me think of all kind of > > > horror > > > >> > scenarios this could/would lead to. > > > >> > > > > >> > So my question is, what would be the smartest way to handle that? > > > Adjust > > > >> > all test cases, so they no longer rely on full equality? Fix > > > >> > (de)serialization to include parent element ids? Something else? > > > >> > > > > >> > *Example of a failing test:* > > > >> > > > > >> > [ERROR] SerializationTest$GryoV1d0Test.shouldSerializeTree:254 > The > > > >> > objects differ > > > >> > expected: > > > >> > > > > >> > > > > >> > > > > > > org.apache.tinkerpop.gremlin.process.traversal.step.util.Tree<{v[1]={v[2]={vp[name->vadas]={}}, > > > >> > v[3]={vp[name->lop]={}}, v[4]={vp[name->josh]={}}}}> > > > >> > but was: > > > >> > > > > >> > > > > >> > > > > > > org.apache.tinkerpop.gremlin.process.traversal.step.util.Tree<{v[1]={v[2]={vp[name->vadas]={}}, > > > >> > v[3]={vp[name->lop]={}}, v[4]={vp[name->josh]={}}}}> > > > >> > > > > >> > > > > >> > Crazy thought: We could just change the tests to compare the > > > toString() > > > >> > versions of results.... > > > >> > > > > >> > Cheers, > > > >> > Daniel > > > >> > > > > >> > > > > > > > > > >
