Follow up on #1640 - Add ExecutionInfo to RequestTracker callbacks #1949
Conversation
| /** | ||
| * @deprecated This method only exists for backward compatibility. Override {@link | ||
| * #onSuccess(Request, long, DriverExecutionProfile, Node, String)} instead. | ||
| * Invoked each time a request succeeds. |
There was a problem hiding this comment.
I have reordered methods in the interface, so that most recent versions of all distinct methods are placed first. This shall give a better view on their current state.
lukasz-antoniak
force-pushed
the
PR-1640
branch
from
51fceda to
2d8b5f9
Compare
tolbertam
left a comment
tolbertam
left a comment
There was a problem hiding this comment.
Was tentatively +1 on #1640, and I think your changes are also good. I like that you retained the original commit, made this easier to review and will also allow us to attribute the change to the original contributor.
The only thing I think we should change is just to add default implementations for getExecutionProfile on the ExecutionInfo interfaces just to retain API compatibility. With that changed I would be a +1.
core/revapi.json
Outdated
| "justification": "Upgrade jackson-databind to 2.13.4.1 to address CVEs, API change cause: https://github.com/FasterXML/jackson-databind/issues/3419" | ||
| }, | ||
| { | ||
| "code": "java.method.addedToInterface", |
There was a problem hiding this comment.
I suppose this would not be a backwards compatible change that we are making in a minor release, which would break the documented following of semver.
I think it would probably be ok to just make default implementations that return null, add a @Nullable annotation, and document that callers should consider use of a custom ExecutionInfo. We could mark removing the default implementation for whenever we next do a major release.
I think this would be pragmatic as it would avoid possibly breaking any third party libraries and requiring another release of the library that the user may not have control of. I'll admit that this seems rather unlikely but I think we should do everything we can to keep semver even if it is somewhat non-optimal.
| } | ||
| } catch (Throwable error) { | ||
| setFinalError(callback.statement, error, callback.node, -1); | ||
| ExecutionInfo executionInfo = defaultExecutionInfo(callback, -1).build(); |
There was a problem hiding this comment.
I know that it worked this way before, but it seems odd to me that we would use -1 for the execution here instead of callback.execution. I guess it's good to leave it this way for consistency like you did, but maybe idea here is "Something unpredictable unexpected happened here that we can't blame on the request itself.
lukasz-antoniak
force-pushed
the
PR-1640
branch
from
2d8b5f9 to
495fdda
Compare
tolbertam
left a comment
tolbertam
left a comment
There was a problem hiding this comment.
Looks great, thank you!
|
Introduced changes:
|
lukasz-antoniak
force-pushed
the
PR-1640
branch
from
9c38b58 to
b45569f
Compare
2 participants
In #1640 we are waiting for author to incorporate our feedback. The intention of this PR is to speed up the process, leaving original author's commit intact and recorded in Git history. Adding
ExecutionInfotoRequestTrackeris needed to implement OpenTelemetry support (JAVA-3148) and addonStartXXX()callbacks (JAVA-3046).