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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
