[GitHub] [tinkerpop] spmallette commented on a diff in pull request #1837: TINKERPOP-2806 Provide method for provider plugins to get notified on script/query processing

[GitBox <gi...@apache.org>]

spmallette commented on code in PR #1837:
URL: https://github.com/apache/tinkerpop/pull/1837#discussion_r1013979828


##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractEvalOpProcessor.java:
##########
@@ -231,9 +233,18 @@ protected void evalOpInternal(final Context ctx, final Supplier<GremlinExecutor>
         final GremlinExecutor.LifeCycle lifeCycle = GremlinExecutor.LifeCycle.build()
                 .evaluationTimeoutOverride(seto)
                 .afterFailure((b,t) -> {
+                    graphManager.onQueryError(msg, t);
                     if (managedTransactionsForRequest) attemptRollback(msg, ctx.getGraphManager(), settings.strictTransactionManagement);
                 })
+                .afterTimeout(b -> {

Review 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?



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org