You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2021/01/18 14:56:11 UTC
[GitHub] [tinkerpop] giovibal opened a new pull request #1380: Use a List of strategies instead of Set
giovibal opened a new pull request #1380:
URL: https://github.com/apache/tinkerpop/pull/1380
### Issue
If we configure 2 strategies with same class, only one is used.
Consider this gremlin query:
```
gremlin> graph.traversal().withStrategies(PartitionStrategy.build().partitionKey("_partition").writePartition("a").readPartitions("a").create(), PartitionStrategy.build().partitionKey("_partition").writePartition("b").readPartitions("b").create()).strategies
==>strategies[ConnectiveStrategy, PartitionStrategy, MatchPredicateStrategy, FilterRankingStrategy, InlineFilterStrategy, IncidentToAdjacentStrategy, AdjacentToIncidentStrategy, RepeatUnrollStrategy, PathRetractionStrategy, CountStrategy, EarlyLimitStrategy, LazyBarrierStrategy, BitsyTraversalStrategy, ProfileStrategy, StandardVerificationStrategy]
```
this is the desired result (2 PartitionStrategy in the list):
```
gremlin> graph.traversal().withStrategies(PartitionStrategy.build().partitionKey("_partition").writePartition("a").readPartitions("a").create(), PartitionStrategy.build().partitionKey("_partition").writePartition("b").readPartitions("b").create()).strategies
==>strategies[ConnectiveStrategy, PartitionStrategy, PartitionStrategy, MatchPredicateStrategy, FilterRankingStrategy, InlineFilterStrategy, IncidentToAdjacentStrategy, AdjacentToIncidentStrategy, RepeatUnrollStrategy, PathRetractionStrategy, CountStrategy, EarlyLimitStrategy, LazyBarrierStrategy, BitsyTraversalStrategy, ProfileStrategy, StandardVerificationStrategy]
```
### Solution
Substitute `Set<TraversalStrategy<?>>` with `List<TraversalStrategy<?>>` in class `org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversalStrategies`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] giovibal commented on pull request #1380: Use a List of strategies instead of Set
Posted by GitBox <gi...@apache.org>.
giovibal commented on pull request #1380:
URL: https://github.com/apache/tinkerpop/pull/1380#issuecomment-763069858
I'll consider to open a DISCUSS after a reassessment of our graph-based services.
I also need to study more about tinkerpop codebase and devforum.
Thank you.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] giovibal commented on pull request #1380: Use a List of strategies instead of Set
Posted by GitBox <gi...@apache.org>.
giovibal commented on pull request #1380:
URL: https://github.com/apache/tinkerpop/pull/1380#issuecomment-762914001
Thank you for your explanation.
For me is more coerent if there is a list of strategies, evaluated in order. For a client perspective, if I pass a List, I expect a List. Othewise must be -at least- an exception during configuration of strategies.
If we can't use 2 PartitionStrategy classes, can extend PartitionStrategy (inside a gremlin plugin) and use a different class-name to circumvent the problem ?
### Our use-case
We use gremlin server with 2 levels of partition strategies: one level to divide config from management data, and inside config we have a partition by tenant.
Since 2015 we are in production with this model and everything works fine.
When we upgraded gremlin from 3.3.4 to v3.4.9, nothing worked anymore for us, and we noticed this change change from List to Set ( 67ddf587a06c2ab27d27c51c659f98116c3ab753 ).
For us this is a breaking change, we need to mantain ad internal fork to release the next version.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1380: Use a List of strategies instead of Set
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1380:
URL: https://github.com/apache/tinkerpop/pull/1380#issuecomment-762922539
I see - sorry about the break. I think we made a mistake with `List` initially and that allowed you to use the API in a way we didn't intend. As we went from 3.3.x to 3.4.x we allow breaking changes in behaviors so in that sense we made the change appropriate to our versioning strategies. It's interesting to know that `PartitionStrategy` actually works the way you are using it, though it is working that way more by blissful accident than intention. While we couldn't make the change you are looking for in 3.4.x it could be arranged for 3.5.0 if we could get some consensus on what would be required. I'd be open to wider discussion on this issue on the dev list if you'd like to start a DISCUSS thread [there](https://lists.apache.org/list.html?dev@tinkerpop.apache.org).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] giovibal closed pull request #1380: Use a List of strategies instead of Set
Posted by GitBox <gi...@apache.org>.
giovibal closed pull request #1380:
URL: https://github.com/apache/tinkerpop/pull/1380
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1380: Use a List of strategies instead of Set
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1380:
URL: https://github.com/apache/tinkerpop/pull/1380#issuecomment-762833834
Hello, this is a fairly serious change in our general semantics around traversal strategies. We recently had some discussion on this topic in the community (i.e. adding multiple strategies of the same type) and have done work on the `master` branch to codify the semantics such that you can only add one strategy of any particular type. I realize that hurts a use case like having multiple `SubgraphStrategy` or as you suggest, usage of multiple `PartitionStrategy` implementations, but I don't think any strategy was designed with that use case in mind and as such is not tested or guaranteed to work in that fashion.
Personally, I think we should avoid further complicating strategies. They are already hard to write, prone to bugs that are difficult to uncover and often do more magic than they should maybe do. If we get into the business of allowing two of the same strategy to execute consecutively it feels like we're not making anything easier.
That said, I'd toyed with the idea of having a `Mergable` interface that could be added to strategies so that at least a strategy of the same type that was "mergable" had some explicit semantics by which they could be combined to one and then executed once on the traversal but it wasn't high enough on anyone's priority list to think through any further than that. It could be a possible idea for the future, but I haven't seen a lot of users (any users really) asking for this. In fact, the only reason it even came up earlier in our discussions was out of idle "what if" discussion with @vtslab on a semi-related issue he was working on.
Anyway, I just thought I'd fill you in on where things are with the sort of functionality you are proposing. Happy to hear any thoughts you might have on the matter.
anyway
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org