Skip to content
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

TINKERPOP-1758 Apply RemoteStrategy before all DecorationStrategy instances #811

Merged
merged 1 commit into from Mar 21, 2018

Conversation

spmallette
Copy link
Contributor

https://issues.apache.org/jira/browse/TINKERPOP-1758

Played around with a few ways to test this, but none seemed especially good. Everything I try to do seems fairly contrived or redundant. In the end, I ended up feeling like it was safe to just rely on the TraversalStrategies sorting system with posts/priors. That's pretty solidly tested so perhaps that is enough...

Here's the result of a quick manual test:

gremlin> g = TinkerGraph.open().traversal().withRemote('conf/remote-graph.properties').withStrategies(SubgraphStrategy.build().vertices(identity()).create())
==>graphtraversalsource[tinkergraph[vertices:0 edges:0], standard]
gremlin> g.V().out().out().values('name').explain()
==>Traversal Explanation
=======================================================================================================================================
Original Traversal                 [GraphStep(vertex,[]), VertexStep(OUT,vertex), VertexStep(OUT,vertex), PropertiesStep([name],value)]

RemoteStrategy               [D]   [RemoteStep(DriverServerConnection-localhost/127.0.0.1:8182 [graph=g])]
SubgraphStrategy             [D]   [RemoteStep(DriverServerConnection-localhost/127.0.0.1:8182 [graph=g])]
ConnectiveStrategy           [D]   [RemoteStep(DriverServerConnection-localhost/127.0.0.1:8182 [graph=g])]
RangeByIsCountStrategy       [O]   [RemoteStep(DriverServerConnection-localhost/127.0.0.1:8182 [graph=g])]
RepeatUnrollStrategy         [O]   [RemoteStep(DriverServerConnection-localhost/127.0.0.1:8182 [graph=g])]
MatchPredicateStrategy       [O]   [RemoteStep(DriverServerConnection-localhost/127.0.0.1:8182 [graph=g])]
FilterRankingStrategy        [O]   [RemoteStep(DriverServerConnection-localhost/127.0.0.1:8182 [graph=g])]
InlineFilterStrategy         [O]   [RemoteStep(DriverServerConnection-localhost/127.0.0.1:8182 [graph=g])]
IncidentToAdjacentStrategy   [O]   [RemoteStep(DriverServerConnection-localhost/127.0.0.1:8182 [graph=g])]
AdjacentToIncidentStrategy   [O]   [RemoteStep(DriverServerConnection-localhost/127.0.0.1:8182 [graph=g])]
PathRetractionStrategy       [O]   [RemoteStep(DriverServerConnection-localhost/127.0.0.1:8182 [graph=g])]
LazyBarrierStrategy          [O]   [RemoteStep(DriverServerConnection-localhost/127.0.0.1:8182 [graph=g])]
TinkerGraphCountStrategy     [P]   [RemoteStep(DriverServerConnection-localhost/127.0.0.1:8182 [graph=g])]
TinkerGraphStepStrategy      [P]   [RemoteStep(DriverServerConnection-localhost/127.0.0.1:8182 [graph=g])]
ProfileStrategy              [F]   [RemoteStep(DriverServerConnection-localhost/127.0.0.1:8182 [graph=g])]
StandardVerificationStrategy [V]   [RemoteStep(DriverServerConnection-localhost/127.0.0.1:8182 [graph=g])]

Final Traversal                    [RemoteStep(DriverServerConnection-localhost/127.0.0.1:8182 [graph=g])]

All tests pass with docker/build.sh -t -n -i

VOTE +1

…tances

Played around with a few ways to test this, but none seemed especially good. Everything I try to do seems fairly contrived or redundant. In the end, I ended up feeling like it was safe to just rely on the TraversalStrategies sorting system with posts/priors. Perhaps that is enough...
@dkuppitz
Copy link
Contributor

VOTE: +1

@robertdale
Copy link
Member

Does this handle custom strategies?

@spmallette
Copy link
Contributor Author

no - this only works by convention @robertdale - i don't think there's a way to do that short of scanning the classpath somehow for "decorations" and ordering them properly. or perhaps something more drastic that mucked with strategy ordering that always placed RemoteStrategy first, but i was hesitant to mess with that too much. i dunno - am i not thinking of another way to implement this?

@robertdale
Copy link
Member

I don't know. Just asking for the record.
VOTE +1

@spmallette
Copy link
Contributor Author

fair enough - i kinda wanted to just fix this problem since it was out there for so long. i'm hesitant to just hardcode the RemoteStrategy as "first" - something feels off about doing that. perhaps we wait until this approach presents as an issue before building lots of new infrastructure to support something more flexible.

@asfgit asfgit merged commit 5288514 into tp32 Mar 21, 2018
@asfgit asfgit deleted the TINKERPOP-1758 branch April 14, 2018 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants