fix: call option deadline timeout unchanged in query method call by karel-rehor · Pull Request #310 · InfluxCommunity/influxdb3-java

karel-rehor · 2025-11-12T16:44:24Z

Closes #301

Proposed Changes

in the implementation queryData method, leave the QueryOptions argument unchanged. Use instead a local copy.

Explanation: The passed QueryOptions object might be reused, in which case mutating a field like Deadline in GrpcOptions could lead to reusing an already expired deadline in future calls.

Add test to verify Deadline is ticking down.
Add test to verify that a QueryOptions object has not been modified. N.B. in support of this test the clone method is overridden to make a deep copy of the original.

N.B. IDE reformatted indentations in the QueryOptionsTest.java file.

Checklist

…e deadlines.

codecov · 2025-11-12T16:46:04Z

Codecov Report

❌ Patch coverage is 75.00000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.22%. Comparing base (4d74671) to head (e169db7).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ava/com/influxdb/v3/client/query/QueryOptions.java	50.00%	1 Missing and 4 partials ⚠️
...nfluxdb/v3/client/internal/InfluxDBClientImpl.java	87.50%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #310      +/-   ##
==========================================
- Coverage   87.48%   87.22%   -0.26%     
==========================================
  Files          20       20              
  Lines        1262     1284      +22     
  Branches      204      213       +9     
==========================================
+ Hits         1104     1120      +16     
- Misses         74       75       +1     
- Partials       84       89       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bednar · 2025-11-13T05:20:32Z

+        GrpcCallOptions grpcCallOptionsCopy = options.grpcCallOptions();
+        if (grpcCallOptionsCopy.getDeadline() == null && config.getQueryTimeout() != null) {
+            grpcCallOptionsCopy = new GrpcCallOptions.Builder()
+                .fromGrpcCallOptions(grpcCallOptionsCopy)
+                .withDeadline(Deadline.after(config.getQueryTimeout().toMillis(), TimeUnit.MILLISECONDS))
+                .build();
+        }
+
+        CallOption[] callOptions = grpcCallOptionsCopy.getCallOptions();


What about simplest solution?

Suggested change

GrpcCallOptions grpcCallOptionsCopy = options.grpcCallOptions();

if (grpcCallOptionsCopy.getDeadline() == null && config.getQueryTimeout() != null) {

grpcCallOptionsCopy = new GrpcCallOptions.Builder()

.fromGrpcCallOptions(grpcCallOptionsCopy)

.withDeadline(Deadline.after(config.getQueryTimeout().toMillis(), TimeUnit.MILLISECONDS))

.build();

}

CallOption[] callOptions = grpcCallOptionsCopy.getCallOptions();

GrpcCallOptions.Builder builder = new GrpcCallOptions.Builder()

.fromGrpcCallOptions(options.grpcCallOptions());

if (config.getQueryTimeout() != null) {

builder.withDeadline(Deadline.after(config.getQueryTimeout().toMillis(), TimeUnit.MILLISECONDS))

}

CallOption[] callOptions = builder.build().getCallOptions();

I think we still need to check in line 421 whether grpcCallOptions in the passed options object, doesn't already have a Deadline defined. options.grpcCallOptions().getDeadline() passed in a single call should supersede config.getQueryTimeout() . But, I'll try something similar now locally.

But, yes I think we can reduce the complexity by getting rid of grpcCallOptionsCopy which was suggested by copilot.

bednar · 2025-11-13T05:23:44Z

+    protected QueryOptions clone() {
+        QueryOptions clone;
+        HashMap<String, String> cloneHeaders = new HashMap<>(this.headers);
+        for (String key : this.headers.keySet()) {
+            cloneHeaders.put(key, this.headers.get(key));
+        }
+        try {
+            clone = (QueryOptions) super.clone();
+        } catch (final CloneNotSupportedException e) {
+            clone = new QueryOptions(this.database, this.queryType, cloneHeaders);
+        }
+        if (this.grpcCallOptions != null) {
+            GrpcCallOptions.Builder grpcOptsBuilder = new  GrpcCallOptions.Builder();
+            if (this.grpcCallOptions.getDeadline() != null) {
+                grpcOptsBuilder.withDeadline(this.grpcCallOptions.getDeadline().offset(0, TimeUnit.MILLISECONDS));
+            }
+            grpcOptsBuilder.withExecutor(this.grpcCallOptions.getExecutor());
+            grpcOptsBuilder.withCompressorName(this.grpcCallOptions.getCompressorName());
+            if (this.grpcCallOptions.getWaitForReady() != null
+                && this.grpcCallOptions.getWaitForReady()) {
+                grpcOptsBuilder.withWaitForReady();
+            }
+            grpcOptsBuilder.withMaxInboundMessageSize(this.grpcCallOptions.getMaxInboundMessageSize());
+            grpcOptsBuilder.withMaxOutboundMessageSize(this.grpcCallOptions.getMaxOutboundMessageSize());
+            clone.grpcCallOptions = grpcOptsBuilder.build();
+        }
+        return clone;


This is mainly for the test queryOptionsUnchangedByCall(). A deep copy of QueryOptions from before when the query() call is made is needed for comparison to the QueryOptions object passed to query() after the call is made, to make sure that the query call did not mutate the passed QueryOptions object in any way.

If this is only for testing, we should remove it. It will be difficult to maintain, because whenever a new property is added to GrpcCallOptions, we would have to update this as well.

OK. I'll move it to a static method in the test class.

…ctical values.

bednar · 2025-11-13T12:25:04Z

+        if (options.grpcCallOptions().getDeadline() == null) {
+            if (config.getQueryTimeout() != null) {
+                builder.withDeadline(Deadline.after(config.getQueryTimeout().toMillis(), TimeUnit.MILLISECONDS));
+            }
+        } else if (options.grpcCallOptions().getDeadline().timeRemaining(TimeUnit.NANOSECONDS) <= 0) {
+            LOG.warning("Received impractical gRPC call options deadline "
+                + options.grpcCallOptions().getDeadline());
+            if (config.getQueryTimeout() != null) {
+                LOG.warning("Using configuration query timeout "
+                    + config.getQueryTimeout() + " as a replacement.");
+                builder.withDeadline(Deadline.after(config.getQueryTimeout().toMillis(), TimeUnit.MILLISECONDS));
+            }
+        }


What about something simple like:

Suggested change

if (options.grpcCallOptions().getDeadline() == null) {

if (config.getQueryTimeout() != null) {

builder.withDeadline(Deadline.after(config.getQueryTimeout().toMillis(), TimeUnit.MILLISECONDS));

}

} else if (options.grpcCallOptions().getDeadline().timeRemaining(TimeUnit.NANOSECONDS) <= 0) {

LOG.warning("Received impractical gRPC call options deadline "

+ options.grpcCallOptions().getDeadline());

if (config.getQueryTimeout() != null) {

LOG.warning("Using configuration query timeout "

+ config.getQueryTimeout() + " as a replacement.");

builder.withDeadline(Deadline.after(config.getQueryTimeout().toMillis(), TimeUnit.MILLISECONDS));

}

}

if (config.getQueryTimeout() != null) {

builder.withDeadline(Deadline.after(config.getQueryTimeout().toMillis(), TimeUnit.MILLISECONDS));

} else if (options.grpcCallOptions().getDeadline() != null) {

LOG.warning("No query timeout configured in ClientConfig, but gRPC call options contains a deadline "

+ options.grpcCallOptions().getDeadline()

+ ". This may lead to unexpected behavior - use setting query timeout in ClientConfig.");

}

?

Updated logic based on further conversation.

… QueryOptions.clone()

bednar · 2025-11-13T13:58:30Z

+        // Assertions.assertThat(thrown).isInstanceOf(FlightRuntimeException.class);
+        // Assertions.assertThat(thrown.getMessage()).matches(".*deadline.*exceeded.*");


Please remove or uncomment this.

bednar

Update JavaDoc in GrpcCallIOptions.Builder.withDeadline that the preferred way is set deadline globally.

bednar

LGTM 🚀

karel-rehor added 3 commits November 12, 2025 15:31

fix: mutating QueryOptions arg in query method can lead to 0 and stal…

c517a0b

…e deadlines.

chore: cleanup lint

2452b09

tests: add test of QueryOptions.clone and comparison.

6b4ebfe

karel-rehor requested review from NguyenHoangSon96, bednar and jansimonb November 12, 2025 16:44

karel-rehor added 3 commits November 12, 2025 17:50

docs: update CHANGELOG.md

743a8aa

tests: additional QueryOptions test

d5239c6

tests: add assert

3e4f4be

bednar requested changes Nov 13, 2025

View reviewed changes

karel-rehor added 2 commits November 13, 2025 10:05

chore: remove debug messages from test

924e01c

chore: refactor gRPC Deadline checks and add logger warning for impra…

178f1bb

…ctical values.

bednar requested changes Nov 13, 2025

View reviewed changes

chore: clarify logic of choosing Deadline value, update tests, remove…

719e8b1

… QueryOptions.clone()

karel-rehor requested a review from bednar November 13, 2025 13:48

bednar requested changes Nov 13, 2025

View reviewed changes

karel-rehor added 2 commits November 13, 2025 15:08

chore: remove commented assertions.

bf987dd

docs: update Javadoc with preferred Deadline approach.

e169db7

karel-rehor requested a review from bednar November 13, 2025 14:32

bednar approved these changes Nov 13, 2025

View reviewed changes

karel-rehor merged commit 5e559a7 into main Nov 13, 2025
7 of 9 checks passed

karel-rehor deleted the fix/CallOptionDeadlineTimeout branch November 13, 2025 16:23

		// Assertions.assertThat(thrown).isInstanceOf(FlightRuntimeException.class);
		// Assertions.assertThat(thrown.getMessage()).matches(".deadline.exceeded.*");

Conversation

karel-rehor commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Checklist

Uh oh!

codecov Bot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karel-rehor Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karel-rehor Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

bednar left a comment

Choose a reason for hiding this comment

Uh oh!

bednar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

karel-rehor commented Nov 12, 2025 •

edited

Loading

codecov Bot commented Nov 12, 2025 •

edited

Loading

karel-rehor Nov 13, 2025 •

edited

Loading

karel-rehor Nov 13, 2025 •

edited

Loading