-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
feature request: Neptune transaction support #10823
Comments
Welcome to LocalStack! Thanks for reporting your first issue and our team will be working towards fixing the issue for you or reach out for more background information. We recommend joining our Slack Community for real-time help and drop a message to LocalStack Pro Support if you are a Pro user! If you are willing to contribute towards fixing this issue, please have a look at our contributing guidelines and our contributing guide. |
Hi @tsanton, I have looked into the issue. While there appears to have been more changes to the overall gremlin server that makes the simple lifting of a couple .jar files more complicated than it seems, there might be another solution that could work in the interim. We could create a configuration variable like Looking at the release notes, I am not seeing any major deprecation going from 3.6 to 3.7. The other major difference is listed under Let me know if you see any other complications I am missing. |
Hi @cloutierMat and thank you for your expedient reply! Personally I would love that solution. Though I'm not a Graph expert, I'd say I'm a layman and an enthusiast. With those restrictions in mind I can think of one potential issue: though deprecations are not a major issue, improvements on existing functionality from 3.6.x to 3.7.x can present behaviour problems (specifically when it comes to MergeV and MergeE). This GitHub issue explains one potential pitfall in depth. In short it's related to the option of specifying property cardinality during merge (can do in 3.7, not possible in 3.6). If we implement the solution you suggest I would imagine the Localstack MergeE/MergeV behaviour with property cardinality would be 👍, whereas if you fired this at AWS Neptune you'd get a solid 👎. In my view I much prefer the (single thread) transaction support with a LocalStack note "FYI: Do not...." over the non-transactional server as is. Further I'm willing to bet a beer and a nacho (if you're ever somewhere in mid to north Europe) that Neptune 1.3.2.0 (due in x-3? months) will support >=3.7.x so this sort of ameliorates the issue. (Even)Further AWS Neptune docs clearly state: "[test your upgrades on real life instances before you roll out your new bundle of joy]" which gives us some leeway when implementing this behaviour change. Thanks for you time and support @cloutierMat! /T |
Thanks for pointing that out! I will keep an eye for it, and document my findings in the docs. |
Hi @tsanton, The feature has now been implemented and you can pull the latest docker image to start using it. Information on how to enable it is in our Documentation. When creating your Neptune instance you should see the following message in the logs indicating that transactions are enabled.
As far as the MergeV/MergeE behaviour, from the few samples I have tried, 3.6.2 queries seem to be working as expected. Let us know how it goes. |
Hi @cloutierMat, and thanks for the swift implementation! I'll get on to refactoring my fixture over the weekend/early next week. I'll get back to you with the results when that is done. Have a great weekend when that time comes :) /T |
Hi again @cloutierMat! I've now had the time to look over it. Firstly the potential issue. I have two verticies (V1 and V2) joined by an edge (LinkedE): V1 -> LinkedE -> V2. This LinkedE has one property: Active (T/F). This property is being updated depending on, you guessed it: the relationship being active or not. The issue here, to me, seems to be the default cardinality. Now when I run the update test I get a failing test and I query out this result (edge properties):
Though this docs is for vertex cardinality, is states that only single or set is supported. I think it's safe to assume that it would be the same for edges? Unfortuntely I'm only in dev so I don't have neptune instance up just yet so I can't verify this. The next "odd" behaviour is related to "implicit" transactions. var g = await traversalFactory.GetClient(tenantId);
var tx = g.Tx();
if (traversalFactory.SupportsTransaction()) g = tx.Begin();
........
if (hasTx && tx is not null)
{
await query.Promise(traversal => traversal.Iterate(), ct);
await tx.CommitAsync(ct);
}
else
{
await query.Promise(traversal => traversal.Iterate(), ct);
} With other words it seems that an entire batch of steps is being given the transactional treatment (i.e. all or nothing). I'm then guessing that the explisit transaction usage is for multiple iterates? Further: is this in accordance with the behaviour of Neptune? |
Hi @tsanton, Awesome that most of your tests are passing now 🔥 🚀 I am a little confused by the errors you are seeing and I would love for you to give me more details on your setup and sample queries to reproduce. But here are my thoughts for the 2 points you bring up. Edge Property CardinalityI am not sure how you could get a See Apache Docs. Point two states Please provide a sample code as to how you are achieving this! 😄 Implicit TransactionLet me know if I understand you correctly. you are performing something like. t = g.add_v("person").as_("p")
t.add_e("knows").from_("p").to("invalidId")
result = t.iterate() The expected behaviour is that the So transaction are only making a difference when multiple terminal steps have occurred in the transaction. See last example on this page. |
In terms of the In terms of the second error I partially have to say sorry. I was way to quick to jump the gun (having debugged the issue for 30 minutes on top of being jetlagged). To re-summarize the issue: in short this is what I'm doing:
First MergeV is the root:
Second MergeV has a
The finally it's the MergeE:
In this case it's the second MergeV that "blows up" because (I think) the MergeV step assumes a default cardinality of A fix here, I think, would be to set the Hope this is a bit more enlightening, but as I've only had half a night of sleep I do reserve the right to (though trying to avoid) making another blunder :) /T |
Glad to see transactions are working as expected. As for the cardinality issue. I have pushed another fix last Friday that set the default Cardinality to And be aware, when using edit: the default behavior is set, which is what you are observing. The 3.6 syntax to specify Cardinality is still valid. g.add_v("p").property("name", "john").next()
g.merge_v({T.label: "p", "name": "john"}).option(
Merge.on_match,
__.side_effect(__.property(Cardinality.single, "name", "john2")).constant({}),
).next()
# results in
{<T.id: 1>: 'b934895b-ffee-40c7-8d35-f55a982a6c90', <T.label: 4>: 'p', 'name': ['john2']} |
I'm sorry @cloutierMat but this you're going to have to run by me one more time. This is not the behaviour I'm seing at the moment -> I'm having exceptions thrown because the serialiser is getting a List when it expects a single bool. To be clear: I'm not observing set (with Further, my understand ing that in 3.6.x syntax you cannot specify cardinality -> this was first introduced in 3.7.x here (though a bit down -> look for |
A set is a list with no repeated element. It only matters when adding property to an element. When querying, your access pattern will determine if you get a string or a list. ie. 3.7 introduce The below examples return the same value for ls-3.6, ls-transaction enabled and Neptune-1.3.0.0 vertex = g.addV("player").property("card", "ace")
g.merge_v({T.id: vertex.id})
.option(Merge.on_match, __.side_effect(property("card", "king")).constant({}))
.valueMap("card")
.next()
## "card": ["ace", "king"]
vertex = g.addV("player").property("card", "ace")
g.merge_v({T.id: vertex.id})
.option(Merge.on_match, __.side_effect(property(Cardinality.set, "card", "king")).constant({}))
.valueMap("card")
.next()
## "card": ["ace", "king"]
vertex = g.addV("player").property("card", "ace")
g.merge_v({T.id: vertex.id})
.option(Merge.on_match, __.side_effect(property(Cardinality.single, "card", "king")).constant({}))
.valueMap("card")
.next()
## "card": ["king"] |
Okay, many thanks @cloutierMat :) So, just to reiterate: when using the MergeV/E-pattern Neptune will assume default cardinality (when not specified, and specifications are not possible < 3.7) which is set. So what I'm seeing now on my MergeV with update options: This
turning into this:
is the expected behavour in Neptune? If that the case then guess I'm going to have to rewrite my "merger" to run a OnUpdate SideEffect with property cardinality instead :) Thanks for your great effort enhancing Localstack Neptune -> it's a major enhancement having a more on pair behaviour from the local emulator! |
Christmas came early this year :) |
Awesome I will be working on support for it hopefully next week. It should be fairly straight forward, but some patches will need updating as we currently provision 3.7.2 for transaction and the new Neptune engine supports up to 3.7.1. As for your example, it seems to me like you are on the right path. I remember it being the major issue with On a side note, the |
I just spent the last hours refactoring the solution and I just made 328/328 green ones with the usage of Cardinality:
There's also a little tweak that must be done if it's a collection (i.e. set above): As I'm sure you know you can only add the first value in the merge and then must add additional values by the .property logic: properties.Where(p => p.Value is CardinalityValue val && Cardinality.Set.Equals(val.Cardinality))
.SelectMany(p => (((CardinalityValue)p.Value).Value as IList ?? new List<object>()).Cast<object>()
.Select(value => new { p.Key, Value = value }))
.ToList()
.ForEach(set => query.Property(Cardinality.Set, set.Key, set.Value)); It's not beautiful, but it works as a charm! In terms of you side note, I'm on northern Europes thinest ice trying to answer this so.. Take it with a bucket of salt. Hope that answers your question :) All in all I'm a happy camper and I guess this issue can be closed! /T |
Awesome, I am glad all is working good for you, and thank you for the extra insight. I was wondering where that cardinality property was coming from, it makes more sense now that it comes from your serializer. I will close this issue and don't hesitate to open a new one should you find more parity issue with Neptune! |
Is there an existing issue for this?
Feature description
Basic transactions were supported in Tinkerpop 3.7.0 by introducing a ´TinkerTransactionGraph´ class. The config for it can be found here.
I'm hoping it's possible for Localstack Neptune to support transactions.
🧑💻 Implementation
I know AWS support Gremlin 3.6.2 <= x <= 3.6.5 at the moment, and that Localstack is running a Gremlin Server behind the scenes. In terms of implementation I'm wondering if it's possible to implement the server config that's linked above by copying the 3.7.X jar and pointing to that transaction class? It would be extremely nice to be able to test locally with transactions, even when limited to single thread processing.
Anything else?
No response
The text was updated successfully, but these errors were encountered: