Limit calls to Conversions.resolveExecutionProfile #1623

chibenwa · 2023-01-22T07:02:42Z

Those repeated calls account for a non-negligible portion of my application
CPU (0.6%) and can definitly be a final field so that it gets resolved only
once per CqlRequestHandler.

lukasz-antoniak · 2024-05-31T10:59:51Z

Do you mind closing #1622, as from what I see, this PR contains those changes?

chibenwa · 2024-05-31T12:12:14Z

👍

lukasz-antoniak · 2024-05-31T12:13:30Z

I think you closed the wrong PR. This one is mostly fine :).

chibenwa · 2024-05-31T13:26:45Z

I forced push this PR in order to remove the now merged commit.

Sorry for the confusion.

chibenwa · 2024-05-31T13:32:27Z

For the record #1622 being merge the remaining resolveExecutionProfile only accounts for .07% of Apache James CPU footprint.

According to me it would fall under the optimization ratio...

Shall I close this PR?

lukasz-antoniak · 2024-05-31T13:47:09Z

Any optimisation is fine. The changes you proposed make code cleaner and the initialisation of execution profile easier to follow. Let us just address one comment that I had regarding the reference in callback class.

absurdfarce

Nice work @chibenwa; overall I like where this is headed. I do think we need to sort out a few things w.r.t. code duplication and consistent use of the new methods throughout the driver code but neither of those should be enormous changes in light of what's already been done here.

absurdfarce · 2024-06-17T19:45:52Z

core/src/main/java/com/datastax/oss/driver/internal/core/cql/Conversions.java

+        ? executionProfile.getBoolean(DefaultDriverOption.REQUEST_DEFAULT_IDEMPOTENCE)
+        : requestIsIdempotent;
+  }
+


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:

public static boolean resolveIdempotence(Request request, InternalDriverContext context) { return resolveIdempotence(request, resolveExecutionProfile(request, context)); }

Similar argument for resolveRequestTimeout() as well.

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.

Ok I proposed two fixups for implementing those suggested changes.

Since resolving an DriverExecutionProfile from an InternalDriverContext is expensive enough, I agree with marking the existing methods with Deprecated and maybe even removing them in the next minor version (as the project does not ensure compatibility between versions (reference), but I think it may just be best to keep these methods deprecated.

absurdfarce · 2024-06-17T22:37:45Z

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);


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) 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.

That is no longer the case with this change

Sure. I'm expecting request to be short lived (5 second ish) and assumed that we only needed the configuration to eventually be propagated.

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.

👍, I think it is good to call out, and that this change is more desirable (on the fly changes should only apply to subsequent requests, not in flight ones), but in any case I don't think it good to rely on the behavior working either way.

tolbertam

This looks good to me, thanks @chibenwa! Just a few tiny nits in the Conversions overloaded methods, with those cleaned up I'd be eager to add my 👍.

core/src/main/java/com/datastax/oss/driver/internal/core/cql/Conversions.java

tolbertam · 2024-06-19T21:54:00Z

core/src/main/java/com/datastax/oss/driver/internal/core/cql/Conversions.java

+        ? executionProfile.getBoolean(DefaultDriverOption.REQUEST_DEFAULT_IDEMPOTENCE)
+        : requestIsIdempotent;
+  }
+


Since resolving an DriverExecutionProfile from an InternalDriverContext is expensive enough, I agree with marking the existing methods with Deprecated and maybe even removing them in the next minor version (as the project does not ensure compatibility between versions (reference), but I think it may just be best to keep these methods deprecated.

core/src/main/java/com/datastax/oss/driver/internal/core/cql/Conversions.java

tolbertam · 2024-06-19T22:04:29Z

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);


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.

👍, I think it is good to call out, and that this change is more desirable (on the fly changes should only apply to subsequent requests, not in flight ones), but in any case I don't think it good to rely on the behavior working either way.

chibenwa · 2024-06-20T07:33:45Z

Hi!

Thanks for the helpfull reviews.

I implemented the requested changes in the latest fixup.

Best regards,

Benoit

tolbertam

Changes look great, thanks @chibenwa !

absurdfarce · 2024-06-24T00:49:43Z

Agree with @tolbertam; I'm pretty pleased with where this landed. I just kicked off a Jenkins build to make sure there aren't any unexpected test regressions or anything but unless we see some trouble in those results I think we're there.

I'll post the results here when the build finishes.

absurdfarce · 2024-06-24T03:41:20Z

Bah, build failed due to a code formatting issue. @chibenwa can you run the following:

mvn com.coveo:fmt-maven-plugin:format

and commit the result to this branch?

absurdfarce · 2024-07-01T16:39:09Z

Awesome, thanks @chibenwa! New Jenkins build kicked off, will post results for this one upon completion.

absurdfarce · 2024-07-01T18:26:38Z

Jenkins build finished with only a single error which is pretty clearly a one-off failure (only failed on a single Java version and consists of one missing value in a set of a million). Might need to look at that just a bit more to see if we can repro but this isn't something we've seen often... and there's no reason to believe it's connected to anything in this change.

Per-Commit / Matrix - SERVER_VERSION = 'dse-6.8.30', JABBA_VERSION = '[email protected]' / Execute-Tests / com.datastax.oss.driver.api.core.uuid.UuidsTest.should_generate_unique_random_uuids_Random
...
Error Message
Expected size: 1000000 but was: 999999 in:
[e4182c8a-d3f8-463e-91c8-15320dc8d466,
    0ff0a789-10e8-4060-a43c-6a99bc448a19,
    9c5c8d25-86c9-463d-9605-9f9ceed036ce,
    4e77a939-5366-4290-899a-f5ce18ab9248,
    ac73d701-6ed8-4451-8539-249a0f92ffd9,

absurdfarce · 2024-07-01T18:27:19Z

Given the results above I'm adding my approval to @tolbertam's. @chibenwa can you consolidate these changes down to a single commit (as discussed in the last PR) so that we can get this merged?

Thanks!

Those repeated calls account for a non-negligible portion of my application CPU (0.6%) and can definitly be a final field so that it gets resolved only once per CqlRequestHandler.

chibenwa · 2024-07-02T07:17:06Z

Here you are

absurdfarce · 2024-07-02T15:50:09Z

This looks great, thanks for all your work on this @chibenwa!

Those repeated calls account for a non-negligible portion of my application CPU (0.6%) and can definitly be a final field so that it gets resolved only once per CqlRequestHandler. patch by Benoit Tellier; reviewed by Andy Tolbert, and Bret McGuire reference: #1623

* CASSANDRA-19635: Run integration tests with C* 5.x patch by Lukasz Antoniak; reviewed by Andy Tolbert, and Bret McGuire for CASSANDRA-19635 * CASSANDRA-19635: Configure Jenkins to run integration tests with C* 5.x patch by Lukasz Antoniak; reviewed by Bret McGuire for CASSANDRA-19635 * update badge URL to org.apache.cassandra/java-driver-core * Limit calls to Conversions.resolveExecutionProfile Those repeated calls account for a non-negligible portion of my application CPU (0.6%) and can definitly be a final field so that it gets resolved only once per CqlRequestHandler. patch by Benoit Tellier; reviewed by Andy Tolbert, and Bret McGuire reference: apache#1623 * autolink JIRA tickets in commit messages patch by Stefan Miklosovic; reviewed by Michael Semb Wever for CASSANDRA-19854 * Don't return empty routing key when partition key is unbound DefaultBoundStatement#getRoutingKey has logic to infer the routing key when no one has explicitly called setRoutingKey or otherwise set the routing key on the statement. It however doesn't check for cases where nothing has been bound yet on the statement. This causes more problems if the user decides to get a BoundStatementBuilder from the PreparedStatement, set some fields on it, and then copy it by constructing new BoundStatementBuilder objects with the BoundStatement as a parameter, since the empty ByteBuffer gets copied to all bound statements, resulting in all requests being targeted to the same Cassandra node in a token-aware load balancing policy. patch by Ammar Khaku; reviewed by Andy Tolbert, and Bret McGuire reference: apache#1620 * JAVA-3167: CompletableFutures.allSuccessful() may return never completed future patch by Lukasz Antoniak; reviewed by Andy Tolbert, and Bret McGuire for JAVA-3167 * ninja-fix Various test fixes * Run integration tests with DSE 6.9.0 patch by Lukasz Antoniak; reviewed by Bret McGuire reference: apache#1955 * JAVA-3117: Call CcmCustomRule#after if CcmCustomRule#before fails to allow subsequent tests to run patch by Henry Hughes; reviewed by Alexandre Dutra and Andy Tolbert for JAVA-3117 * JAVA-3149: Support request cancellation in request throttler patch by Lukasz Antoniak; reviewed by Andy Tolbert and Chris Lohfink for JAVA-3149 * Fix C* 3.0 tests failing on Jenkins patch by Lukasz Antoniak; reviewed by Bret McGuire reference: apache#1939 * Reduce lock held duration in ConcurrencyLimitingRequestThrottler It might take some (small) time for callback handling when the throttler request proceeds to submission. Before this change, the throttler proceed request will happen while holding the lock, preventing other tasks from proceeding when there is spare capacity and even preventing tasks from enqueuing until the callback completes. By tracking the expected outcome, we can perform the callback outside of the lock. This means that request registration and submission can proceed even when a long callback is being processed. patch by Jason Koch; Reviewed by Andy Tolbert and Chris Lohfink for CASSANDRA-19922 * Annotate BatchStatement, Statement, SimpleStatement methods with CheckReturnValue Since the driver's default implementation is for BatchStatement and SimpleStatement methods to be immutable, we should annotate those methods with @CheckReturnValue. Statement#setNowInSeconds implementations are immutable so annotate that too. patch by Ammar Khaku; reviewed by Andy Tolbert and Bret McGuire reference: apache#1607 * Remove "beta" support for Java17 from docs patch by Bret McGuire; reviewed by Andy Tolbert and Alexandre Dutra reference: apache#1962 * Fix uncaught exception during graceful channel shutdown after exceeding max orphan ids patch by Christian Aistleitner; reviewed by Andy Tolbert, and Bret McGuire for apache#1938 * Build a public CI for Apache Cassandra Java Driver patch by Siyao (Jane) He; reviewed by Mick Semb Wever for CASSANDRA-19832 * CASSANDRA-19932: Allow to define extensions while creating table patch by Lukasz Antoniak; reviewed by Bret McGuire and Chris Lohfink * Fix DefaultSslEngineFactory missing null check on close patch by Abe Ratnofsky; reviewed by Andy Tolbert and Chris Lohfink for CASSANDRA-20001 * Query builder support for NOT CQL syntax patch by Bret McGuire; reviewed by Bret McGuire and Andy Tolbert for CASSANDRA-19930 * Fix CustomCcmRule to drop `CURRENT` flag no matter what If super.after() throws an Exception `CURRENT` flag is never dropped which leads next tests to fail with IllegalStateException("Attempting to use a Ccm rule while another is in use. This is disallowed") Patch by Dmitry Kropachev; reviewed by Andy Tolbert and Bret McGuire for JAVA-3117 * JAVA-3051: Memory leak patch by Jane He; reviewed by Alexandre Dutra and Bret McGuire for JAVA-3051 * Automate latest Cassandra versions when running CI patch by Siyao (Jane) He; reviewed by Mick Semb Wever for CASSJAVA-25 * Refactor integration tests to support multiple C* distributions. Test with DataStax HCD 1.0.0 patch by Lukasz Antoniak; reviewed by Bret McGuire reference: apache#1958 * Fix TableMetadata.describe() when containing a vector column patch by Stefan Miklosovic; reviewed by Bret McGuire for CASSJAVA-2 * Move Apache Cassandra 5.x off of beta1 and remove some older Apache Cassandra versions. patch by Bret McGuire; reviewed by Bret McGuire for CASSJAVA-54 * Update link to Jira to be CASSJAVA Updating the link to Jira. Previously we had a component in the CASSANDRA Jira project but now we have a project for each driver - in the case of Java, it's CASSJAVA. Added CASSJAVA to .asf.yaml patch by Jeremy Hanna; reviewed by Bret McGuire for CASSJAVA-61 * Move DataStax shaded Guava module into Java driver patch by Lukasz Antoniak; reviewed by Alexandre Dutra and Bret McGuire for CASSJAVA-52 * JAVA-3057 Allow decoding a UDT that has more fields than expected patch by Ammar Khaku; reviewed by Andy Tolbert and Bret McGuire reference: apache#1635 * CASSJAVA-55 Remove setting "Host" header for metadata requests. With some sysprops enabled this will actually be respected which completely borks Astra routing. patch by Bret McGuire; reviewed by Alexandre Dutra and Bret McGuire for CASSJAVA-55 * JAVA-3118: Add support for vector data type in Schema Builder, QueryBuilder patch by Jane He; reviewed by Mick Semb Wever and Bret McGuire for JAVA-3118 reference: apache#1931 * Upgrade Guava to 33.3.1-jre patch by Lukasz Antoniak; reviewed by Alexandre Dutra and Bret McGuire for CASSJAVA-53 * Do not always cleanup Guava shaded module before packaging * Revert "Do not always cleanup Guava shaded module before packaging" This reverts commit 5be52ec. * Conditionally compile shaded Guava module * JAVA-3143: Extend driver vector support to arbitrary subtypes and fix handling of variable length types (OSS C* 5.0) patch by Jane He; reviewed by Bret McGuire and João Reis reference: apache#1952 * JAVA-3168 Copy node info for contact points on initial node refresh only from first match by endpoint patch by Alex Sasnouskikh; reviewed by Andy Tolbert and Alexandre Dura for JAVA-3168 * JAVA-3055: Prevent PreparedStatement cache to be polluted if a request is cancelled. There was a critical issue when the external code cancels a request, indeed the cached CompletableFuture will then always throw a CancellationException. This may happens, for example, when used by reactive like Mono.zip or Mono.firstWithValue. patch by Luc Boutier; reviewed by Alexandre Dutra and Bret McGuire reference: apache#1757 * Expose a decorator for CqlPrepareAsyncProcessor cache rather than the ability to specify an arbitrary cache from scratch. Also bringing tests from apache#2003 forward with a few minor changes due to this implementation patch by Bret McGuire; reviewed by Bret McGuire and Andy Tolbert reference: apache#2008 * ninja-fix Using shaded Guava classes for import in order to make OSGi class paths happy. Major hat tip to Dmitry Konstantinov for the find here! * Changelog updates for 4.19.0 * [maven-release-plugin] prepare release 4.19.0 * [maven-release-plugin] prepare for next development iteration * Specify maven-clean-plugin version Sets the version to 3.4.1 in parent pom. Having it unspecified causes the following warning: ``` [WARNING] Some problems were encountered while building the effective model for com.scylladb:java-driver-guava-shaded:jar:4.19.0.0-SNAPSHOT [WARNING] 'build.plugins.plugin.version' for org.apache.maven.plugins:maven-clean-plugin is missing. @ line 97, column 15 [WARNING] [WARNING] It is highly recommended to fix these problems because they threaten the stability of your build. [WARNING] [WARNING] For this reason, future Maven versions might no longer support building such malformed projects. [WARNING] ``` * Install guava-shaded before running core's compile in CI Without this module available "Build" and "Unit tests" job fail with "package <x> does not exist" or "cannot find symbol" pointing to `[...].shaded.guava.[...]` packages. * Remove exception catch in `prepared_stmt_metadata_update_loopholes_test` Since incorporating "JAVA-3057 Allow decoding a UDT that has more fields than expected" the asuumptions of removed check are no longer valid. * Switch shaded guava's groupId in osgi-tests Switches from `org.apache.cassandra` to `com.scylladb` in `BundleOptions#commonBundles()`. * Typo: increase line's loglevel in CcmBridge --------- Co-authored-by: Lukasz Antoniak <[email protected]> Co-authored-by: Brad Schoening <[email protected]> Co-authored-by: Benoit Tellier <[email protected]> Co-authored-by: Stefan Miklosovic <[email protected]> Co-authored-by: Ammar Khaku <[email protected]> Co-authored-by: absurdfarce <[email protected]> Co-authored-by: Henry Hughes <[email protected]> Co-authored-by: Jason Koch <[email protected]> Co-authored-by: Christian Aistleitner <[email protected]> Co-authored-by: janehe <[email protected]> Co-authored-by: Abe Ratnofsky <[email protected]> Co-authored-by: absurdfarce <[email protected]> Co-authored-by: Dmitry Kropachev <[email protected]> Co-authored-by: janehe <[email protected]> Co-authored-by: Jeremy Hanna <[email protected]> Co-authored-by: SiyaoIsHiding <[email protected]> Co-authored-by: Alex Sasnouskikh <[email protected]> Co-authored-by: Luc Boutier <[email protected]>

chibenwa force-pushed the resolveExecutionContextLess branch from f7418a6 to 7747946 Compare January 26, 2023 14:56

chibenwa closed this May 31, 2024

chibenwa reopened this May 31, 2024

chibenwa force-pushed the resolveExecutionContextLess branch from 7747946 to 80d7c61 Compare May 31, 2024 13:26

absurdfarce self-requested a review June 17, 2024 19:40

absurdfarce requested changes Jun 17, 2024

View reviewed changes

absurdfarce mentioned this pull request Jun 17, 2024

Add ExecutionInfo to RequestTracker methods #1640

Open

tolbertam self-requested a review June 19, 2024 21:39

tolbertam requested changes Jun 19, 2024

View reviewed changes

tolbertam approved these changes Jun 21, 2024

View reviewed changes

absurdfarce approved these changes Jul 1, 2024

View reviewed changes

Limit calls to Conversions.resolveExecutionProfile

679cf6f

Those repeated calls account for a non-negligible portion of my application CPU (0.6%) and can definitly be a final field so that it gets resolved only once per CqlRequestHandler.

chibenwa force-pushed the resolveExecutionContextLess branch from 0aba36d to 679cf6f Compare July 2, 2024 07:17

absurdfarce merged commit 6d7b195 into apache:4.x Jul 2, 2024

Limit calls to Conversions.resolveExecutionProfile #1623

Limit calls to Conversions.resolveExecutionProfile #1623

Uh oh!

Conversation

chibenwa commented Jan 22, 2023

Uh oh!

lukasz-antoniak commented May 31, 2024

Uh oh!

chibenwa commented May 31, 2024

Uh oh!

lukasz-antoniak commented May 31, 2024

Uh oh!

chibenwa commented May 31, 2024

Uh oh!

chibenwa commented May 31, 2024

Uh oh!

lukasz-antoniak commented May 31, 2024

Uh oh!

absurdfarce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tolbertam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chibenwa commented Jun 20, 2024

Uh oh!

tolbertam left a comment

Choose a reason for hiding this comment

Uh oh!

absurdfarce commented Jun 24, 2024

Uh oh!

absurdfarce commented Jun 24, 2024

Uh oh!

absurdfarce commented Jul 1, 2024

Uh oh!

absurdfarce commented Jul 1, 2024

Uh oh!

absurdfarce commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chibenwa commented Jul 2, 2024

Uh oh!

absurdfarce commented Jul 2, 2024

Uh oh!

Uh oh!

absurdfarce commented Jul 1, 2024 •

edited

Loading