lukasz-antoniak commented on PR #1640:
URL:
https://github.com/apache/cassandra-java-driver/pull/1640#issuecomment-2135352720
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?
--
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-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
