absurdfarce commented on code in PR #1623:
URL:
https://github.com/apache/cassandra-java-driver/pull/1623#discussion_r1643368749
##########
core/src/main/java/com/datastax/oss/driver/internal/core/cql/Conversions.java:
##########
@@ -543,21 +543,44 @@ public static boolean resolveIdempotence(Request request,
InternalDriverContext
: requestIsIdempotent;
}
+ public static boolean resolveIdempotence(Request request,
DriverExecutionProfile executionProfile) {
+ Boolean requestIsIdempotent = request.isIdempotent();
+ return (requestIsIdempotent == null)
+ ?
executionProfile.getBoolean(DefaultDriverOption.REQUEST_DEFAULT_IDEMPOTENCE)
+ : requestIsIdempotent;
+ }
+
Review Comment:
It's worth noting that the implication of the change referenced above (and
indeed this entire PR really) is that a user of this API will only see the
benefit of reduced resolution of ExecutionProfiles if they use the new API;
anyone using the old method `(resolveIdempotence(Request,
InternalDriverContext)` will still see just as many resolution operations as
before.
Seems like at a minimum this older method should be marked as deprecated (in
favor of the new one) but it doesn't seem like a huge leap from there to say if
we're going to go down this path let's just remove the old method and make all
callers responsible for managing DriverExecutionProfile as state. This will
require some additional work; based on a quick check it looks like the
DSE-specific request handlers (such as
`com.datastax.dse.driver.internal.core.cql.continuous.ContinuousRequestHandlerBase`
and `com.datastax.dse.driver.internal.core.graph.GraphRequestHandler`) also
use resolveIdempotence(). These calls would also need to be refactored if we
choose to remove the old method calls.
##########
core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandler.java:
##########
@@ -167,7 +168,8 @@ protected CqlRequestHandler(
this.sessionMetricUpdater = session.getMetricUpdater();
this.timer = context.getNettyOptions().getTimer();
- Duration timeout = Conversions.resolveRequestTimeout(statement, context);
+ this.executionProfile =
Conversions.resolveExecutionProfile(initialStatement, context);
+ Duration timeout = Conversions.resolveRequestTimeout(statement,
executionProfile);
Review Comment:
This change does introduce a change in behaviour that I think is worth
calling out and talking about. I don't think it amounts to a substantial
change so I suspect it'll be just fine... but it did seem worth discussing.
The current impl of 4.x resolves execution profiles in real time. Indeed
that's kind of the point of this entire change; we want to do this resolution a
lot less. The effect of resolving these profiles in real time means that any
changes we might make to the configuration (say via the driver's support for
[manual config
reloading](https://github.com/apache/cassandra-java-driver/tree/4.x/manual/core/configuration#manual-reloading))
between sending the message (at the creation of CqlRequestHandler) and receipt
of the response (via the callback) will be reflected in the callback handling.
That is no longer the case with this change; since we resolve a single
ExecutionProfile at CqlRequestHandler creation and call it directly in the
callback any configuration changes won't be reflected in the behaviour of the
callback.
To be very clear: I'm not saying this is a problem. There's a decent
argument to be made that an entire request/response cycle should have a
constant config in play, something which wasn't present before and is now. So
there's a non-trivial argument that this approach is actually _better_. Either
way it is a _difference_, however, so it seemed worthwhile spending a moment on
it.
##########
core/src/main/java/com/datastax/oss/driver/internal/core/cql/Conversions.java:
##########
@@ -543,21 +543,44 @@ public static boolean resolveIdempotence(Request request,
InternalDriverContext
: requestIsIdempotent;
}
+ public static boolean resolveIdempotence(Request request,
DriverExecutionProfile executionProfile) {
+ Boolean requestIsIdempotent = request.isIdempotent();
+ return (requestIsIdempotent == null)
+ ?
executionProfile.getBoolean(DefaultDriverOption.REQUEST_DEFAULT_IDEMPOTENCE)
+ : requestIsIdempotent;
+ }
+
Review Comment:
This approach provides a nice optimization for callers which have already
resolved the DriverExecutionProfile but it does lead to a lot of code
duplication. Seems like if we pursue this path `resolveIdempotence(Request,
InternalDriverContext)` should be replaced with:
```java
public static boolean resolveIdempotence(Request request,
InternalDriverContext context) {
return resolveIdempotence(request, resolveExecutionProfile(request,
context));
}
```
Similar argument for resolveRequestTimeout() as well.
--
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
