datastax
Add ExecutionInfo to RequestTracker methods
RequestTracker implementations may want to use the ExecutionInfo object e.g logging request/response size, etc.
Thanks for the PR @ajweave!
Have you signed the Contributor License Agreement for contributions to DataStax open source projects? If not you can find it at https://cla.datastax.com/. Thanks!
@ajweave Just so I have this straight in my own head, here's how I understand the relationship between this PR and the ask on JAVA-3046. This PR clearly extends the callbacks for request success and failure to include the extra ExecutionInfo object while JAVA-3046 asks for new callbacks all together for the start of a request. Do I have the contours basically right there?
@absurdfarce the current behavior of RequestTracker providing success/failure after the fact is sufficient for my needs. The only additional thing I require is the ExecutionInfo. Making RequestTracker support finer-grained lifecycle events is ok with me as long as I can get my hands on ExecutionInfo in onSuccess.
Now that the donation to Apache is complete, I'd like to move forward with this change. @hhughes , @absurdfarce , please take a look.
+1 , this would be a great addition , allowing more granular access to execution metrics.
Sorry for the delay. I will get this done tomorrow. I appreciate the attention here.
Good work @ajweave, sorry to keep you waiting. Two things to consider from my end. Feel free to disagree since both are more of coding style category.
In request handlers (e.g. CqlRequestHandler), we very frequently do two things with same arguments:
trackNodeError(node, error, NANOTIME_NOT_MEASURED_YET);
setFinalError(statement, error, node, execution);
Both methods create their own instance of DefaultExecutionInfo internally. Shall we create the ExecutionInfo outside and pass it as parameter? Some of the other parameters like statement or node, could be taken from ExecutionInfo and not passed individually. When we use NoopRequestTracker, we do not need to create DefaultExecutionInfo inside trackNodeError, but still it would be needed in setFinalError, so we shall not degrade performance in this case.
All of this can be done, because we decide to construct ExecutionInfo for all exceptional executions, not only once resulting in DriverException. Anyway, most of the time we would see DriverException being raised.
DefaultExecutionInfo has few optional parameters, but from what I see, we use the constructor in different contexts. What do you think about making this constructor private and expose create(...) methods to indicate what type of DefaultExecutionInfo we are going to pass? Also, would move pagingState calculation inside (currently code copied across different request handlers).
/** Use when driver failed to send the request to the server, or we did not receive any response. */
public static DefaultExecutionInfo create(Request request, Node coordinator, int speculativeExecutionCount,
int successfulExecutionIndex, List<Map.Entry<Node, Throwable>> errors,
DefaultSession session, InternalDriverContext context,
DriverExecutionProfile executionProfile) {
return create(request, coordinator, speculativeExecutionCount, successfulExecutionIndex, errors,
null, null, true, session, context, executionProfile);
}
/** Use when driver received the response, but operation result remains unknown. */
public static DefaultExecutionInfo create(Request request, Node coordinator, int speculativeExecutionCount,
int successfulExecutionIndex, List<Map.Entry<Node, Throwable>> errors,
Frame frame, DefaultSession session, InternalDriverContext context,
DriverExecutionProfile executionProfile) {
return create(request, coordinator, speculativeExecutionCount, successfulExecutionIndex, errors,
null, frame, true, session, context, executionProfile);
}
public static DefaultExecutionInfo create(Request request, Node coordinator, int speculativeExecutionCount,
int successfulExecutionIndex, List<Map.Entry<Node, Throwable>> errors,
Result resultMessage, Frame frame, boolean schemaInAgreement,
DefaultSession session, InternalDriverContext context,
DriverExecutionProfile executionProfile) {
final ByteBuffer pagingState =
(resultMessage instanceof Rows) ? ((Rows) resultMessage).getMetadata().pagingState : null;
return new DefaultExecutionInfo(request, coordinator, speculativeExecutionCount, successfulExecutionIndex, errors,
pagingState, frame, schemaInAgreement, session, context, executionProfile);
}
I was even thinking of changing the three create(...) methods into different names like, of, ofClientError, ofServerError. Thoughts?
Edit:
Yet another option would be to introduce builder pattern for DefaultExecutionInfo class and might be a nicest solution.
public static Builder builder(
Request request,
Node coordinator,
int speculativeExecutionCount,
int successfulExecutionIndex,
List<Map.Entry<Node, Throwable>> errors,
DefaultSession session,
InternalDriverContext context,
DriverExecutionProfile executionProfile) {
return new Builder(
request,
coordinator,
speculativeExecutionCount,
successfulExecutionIndex,
errors,
session,
context,
executionProfile);
}
public static class Builder {
private final Request request;
private final Node coordinator;
private final int speculativeExecutionCount;
private final int successfulExecutionIndex;
private final List<Map.Entry<Node, Throwable>> errors;
private final DefaultSession session;
private final InternalDriverContext context;
private final DriverExecutionProfile executionProfile;
private Result response;
private Frame frame;
private boolean schemaInAgreement = true;
public Builder(
Request request,
Node coordinator,
int speculativeExecutionCount,
int successfulExecutionIndex,
List<Map.Entry<Node, Throwable>> errors,
DefaultSession session,
InternalDriverContext context,
DriverExecutionProfile executionProfile) {
this.request = request;
this.coordinator = coordinator;
this.speculativeExecutionCount = speculativeExecutionCount;
this.successfulExecutionIndex = successfulExecutionIndex;
this.errors = errors;
this.session = session;
this.context = context;
this.executionProfile = executionProfile;
}
public Builder withServerResponse(Result response, Frame frame) {
this.response = response;
this.frame = frame;
return this;
}
public Builder withServerResponse(Frame frame) {
return withServerResponse(null, frame);
}
public Builder withSchemaInAgreement(boolean schemaInAgreement) {
this.schemaInAgreement = schemaInAgreement;
return this;
}
public DefaultExecutionInfo build() {
final ByteBuffer pagingState =
(response instanceof Rows) ? ((Rows) response).getMetadata().pagingState : null;
return new DefaultExecutionInfo(
request,
coordinator,
speculativeExecutionCount,
successfulExecutionIndex,
errors,
pagingState,
frame,
schemaInAgreement,
session,
context,
executionProfile);
}
}
A quick status update here: there's some overlap between the changes in this PR and some changes being proposed in @chibenwa's PR to minimize the number of resolutions of ExecutionProfile in CqlRequestHandler. Since that PR is quite a bit smaller and simpler I'd like to get it in first and then rebase this PR against those changes; doing it the other way around seems like a lot more work :). I've mentioned this relationship to the other committers on ASF Slack so hopefully we can get the other PR in quickly... we'll see how that goes.
We realize that RequestTracker.getErrors was used for "errors from previous coordinators". We want to discuss whether we should still stick to that. I started the conversation in the ASF slack in the "#cassandra-drivers" channel. @ajweave you are welcome to join the conversation.
@ajweave, @tolbertam, I have tried to solve my comments and submitted PR to Andrew's fork: https://github.com/ajweave/java-driver/pull/1. Could you please review the changes and comment? I am not enforcing any of those, just trying cleanup already existing code a little.