TINKERPOP-2806 Provide method for provider plugins to get notified on script/query processing#1837
TINKERPOP-2806 Provide method for provider plugins to get notified on script/query processing#1837tkolanko wants to merge 4 commits intoapache:3.6-devfrom tkolanko:3.6-dev
Conversation
… script/query processing This PR adds 3 new default methods to the GraphManager interface allowing custom GraphManagers to opt into receiving notifications based on the lifecycle of scripts/traversals. AbstractEvalOpProcessor and TraversalOpProcessor get the GraphManager from context and call the lifecycle methods as needed. docker/build.sh -t -i -n passes all tests
| graphManager.onQueryError(msg, t); | ||
| if (managedTransactionsForRequest) attemptRollback(msg, ctx.getGraphManager(), settings.strictTransactionManagement); | ||
| }) | ||
| .afterTimeout(b -> { |
There was a problem hiding this comment.
this is unfortunate. i assume there is no way to get the original time-out exception thrown? do we really have to construct a fresh exception here just for purpose of getting one to onQueryError()?
There was a problem hiding this comment.
In 3e5a38c I refactored all instances of afterTimeout to switch from a Consumer to a BiConsumer so that it can be called with the original exception. It was a smallish change, however, I'm not sure the best way to update the docs to indiciate the signature change and given that this is now a breaking change I think it should be targetted for the next major. I'm not sure how prevalent lifecycle usages are in the wild
There was a problem hiding this comment.
sorry for the delay in reviewing your latest changes. unfortunately, i think the breaking change forces this into 3.7.x rather than 3.6.x. there are several providers who rely on this API. Maybe you could turn it back into a non-breaking change though? Since it is just a callback, you could add the new signature for afterTimeout() as an overload. You could then have GremlinExecutor call both. Deprecate the Consumer version and ensure the javadoc makes clear that users should only utilize one of those two methods as a single timeout triggers them both. Would that work?
There was a problem hiding this comment.
@spmallette In 718c083 I deprecated the use of afterTimeout as a consumer and I had the deprecated method create a new BiConsumer which calls the updated afterTimeout method. I also reverted the GremlinExecutor tests to use the deprecated method to make sure they are still working, I can either move them to the new method or leave things as they are
afterTimeout has changed from a Consumer to a BiConsumer allowing it to accept a throwable when called.
Codecov Report
@@ Coverage Diff @@
## 3.6-dev #1837 +/- ##
=============================================
- Coverage 69.66% 69.43% -0.23%
- Complexity 9308 9346 +38
=============================================
Files 852 875 +23
Lines 38058 41764 +3706
Branches 5620 5624 +4
=============================================
+ Hits 26512 28998 +2486
- Misses 9737 10780 +1043
- Partials 1809 1986 +177
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
3 participants
This PR adds 3 new default methods to the GraphManager interface allowing custom GraphManagers to opt into receiving notifications based on the lifecycle of scripts/traversals.
AbstractEvalOpProcessor and TraversalOpProcessor get the GraphManager from context and call the lifecycle methods as needed.
docker/build.sh -t -i -n passes all tests
I created a proposal thread on https://lists.apache.org/thread/b3k8b4j29gswyk39pl4cfsvd5ofozh4l but there wasn't much interaction, I'm hoping we can get some more discussion from a PR