feat(client): support custom Executor for async operations - #691
Conversation
:java_duke: JaCoCo coverage report
|
|
||||||||||||||
|
HTML test reports are available as workflow artifacts (zipped HTML). • Download: Artifacts for this run |
edeandrea
left a comment
edeandrea
left a comment
There was a problem hiding this comment.
Thanks @Naman-Gururani for the PR! I think it would be better for the Executor to never be @Nullable, except in the builder itself. The null check and setting the default should happen in the DoclingServeClient constructor I think.
|
Thanks @edeandrea, done in 90321a4: the executor is now only |
There was a problem hiding this comment.
Thanks for the work here @Naman-Gururani — the design is what we agreed on in #664 and the plumbing through AsyncOperations -> ConvertOperations / ChunkOperations / toBuilder() looks right. A few things I'd like to see addressed before merging.
One correction to my own advice in #664, and I apologize for the churn: I suggested defaulting to ForkJoinPool.commonPool(). After reviewing the code I think that's the wrong call — please keep the Executor @Nullable all the way down and only pass it to supplyAsync / delayedExecutor when it was actually set. Details in the inline comment on AsyncOperations.
The rest are smaller: test assertions that don't quite prove what the PR claims, doc nit.
One more thing, not inline: the 4-arg public constructors of ConvertOperations and ChunkOperations were replaced rather than overloaded, which is a source- and binary-incompatible change to published public API. Call it out explicitly in the what's-new entry.
|
HTML test reports are available as workflow artifacts (zipped HTML). • Download: Artifacts for this run |
edeandrea
changed the title
edeandrea
left a comment
edeandrea
left a comment
There was a problem hiding this comment.
Thanks @Naman-Gururani — this addresses everything from the last round, and I verified each point against the source rather than the commit messages.
The default-executor fix is exactly right: asyncExecutor is @Nullable end to end and the new supply() / delayed() helpers fall back to the 1-arg CompletableFuture overloads, so the unscreened delayedExecutor(..., commonPool()) hazard is gone. The comment explaining why the common pool isn't equivalent to CompletableFuture's default executor is a nice touch. asyncExecutor as a default method, the restored 4-arg constructor overloads, the Abstract... + Jackson2/Jackson3 test split, the submission counter, and the SUCCESS + chunking scenarios all look good.
I ran it locally at this head: 6 tests pass per Jackson client, DoclingServeApiTests passes, spotlessCheck clean on both modules. I also cross-checked the stubbed paths against the real TaskOperations endpoints — they match.
Three minor things left, none blocking. The Proxy one is the most substantive.
|
HTML test reports are available as workflow artifacts (zipped HTML). • Download: Artifacts for this run |
edeandrea
left a comment
edeandrea
left a comment
There was a problem hiding this comment.
Approving — thank you for this, @Naman-Gururani, and for your patience through several rounds of review.
The final design is exactly right. Keeping asyncExecutor @Nullable end to end and deferring to CompletableFuture's own default when it isn't set means the "nothing changes unless you set it" promise is literally true, including on low-parallelism hosts where commonPool() and CompletableFuture's default executor diverge. You absorbed that redesign after I reversed my own advice from #664, which was my mistake — thanks for taking the rework in stride.
A few things I particularly appreciated:
- The
MinimalBuilderstub does real work. I checked: revertingdefaultback toabstractonDoclingApiBuilder#asyncExecutornow fails at compile time naming the exact method, which is precisely the guarantee we care about for SPI implementors. - The named submission-count constants, with the comment spelling out that the delay step is
delayedExecutorposting to its base executor once the delay elapses. That's the kind of note that saves the next reader twenty minutes. - Following the established test pattern (
Abstract...+ Jackson2/Jackson3) and covering the chunking path as well as conversion.
Verified locally at 2832081: 14/14 tests pass, spotlessCheck clean on both modules, and the stubbed endpoints match the real TaskOperations paths.
One footnote, no action needed and not worth another push: assertThat(this.executions).hasValue(0) in asyncOperationsWorkWithoutCustomExecutor is vacuous — client(null) never wires recordingExecutor into the client, so the counter is 0 no matter how the client behaves. That one came from my suggestion last round, not from anything you got wrong. I'm leaving it in; noting it here only so nobody later mistakes it for a real guard.
Thanks again for a well-executed contribution.
|
Thanks @edeandrea for all the reviews, really appreciate it! Happy to squash the commits before merge if you want. |
If you wouldn't mind that would be great so that the Semantic PR check will pass. I already renamed the PR title accordingly. Once thats done and all the checks pass I'll go ahead and merge. |
Add DoclingApiBuilder.asyncExecutor(Executor) so that async operations (task submission, status polling including the delayed re-polls, and result retrieval) can run on a caller-provided executor. - The builder method is a default method throwing UnsupportedOperationException, so existing builder implementations keep compiling. - The executor stays nullable down to AsyncOperations. When it is not set, the 1-arg supplyAsync and 2-arg delayedExecutor are used, so the behaviour is unchanged. - toBuilder() carries the executor over, and the client never shuts it down. - ConvertOperations, ChunkOperations and AsyncOperations gain constructor overloads taking the executor. Closes docling-project#664 Signed-off-by: Naman Gururani <gururaninaman@gmail.com>
2832081 to
b711e0f
Compare
|
Done, squashed into a single commit with the PR title as the message. |
|
HTML test reports are available as workflow artifacts (zipped HTML). • Download: Artifacts for this run |
|
HTML test reports are available as workflow artifacts (zipped HTML). • Download: Artifacts for this run |
…iProvider (#696) DoclingServeApiBuilderFactory handed out a DoclingApiBuilder, an interface every implementation had to implement. Each new configuration option was either a breaking change or a default method throwing UnsupportedOperationException. The new DoclingServeApiProvider SPI instead receives an immutable DoclingServeApiConfig, a final class owned by docling-serve-api, so options can be added without breaking implementations. - DoclingServeApi.builder() returns a DoclingServeApiBuilder collecting a DoclingServeApiConfig; build() hands it to the single available provider - Providers declare the options they don't honor via unsupportedOptions() (IGNORE, WARN or FAIL), enforced only for explicitly set options - DoclingServeApi gains an abstract config(); an API is copied with api.config().toBuilder() - DoclingServeApiBuilderFactory, DoclingApiBuilder and DoclingServeApi.toBuilder() are deprecated for removal. Legacy factories are still used when no provider is found, but options added after the deprecation, such as asyncExecutor, fail through them - docling-serve-client provides DoclingServeClientProvider. The client builder keeps a DoclingServeApiBuilder instead of duplicating every option, and validates values when they are set - DoclingServeClient.builder() detects Jackson 2 or 3 for client-specific settings; the builder() methods of the Jackson clients are now public, and DoclingServeClientBuilderFactory is deprecated for removal - toBuilder() of the reference client keeps the timeouts and the redirect policy - slf4j-api moves from docling-serve-client to docling-serve-api - Remove the unreleased default DoclingApiBuilder.asyncExecutor() from #691: asyncExecutor is now a DoclingServeApiConfig option - Add a migration guide, and set the release version to 0.7.0 BREAKING CHANGE: DoclingServeApi.builder() now returns a DoclingServeApiBuilder instead of the builder of the implementation, and DoclingServeApi has a new abstract config() method. Signed-off-by: Eric Deandrea <eric.deandrea@ibm.com>
|
@all-contributors add @Naman-Gururani for code |
|
I've put up a pull request to add @Naman-Gururani! 🎉 |
|
@Naman-Gururani your PR actually got me thinking. Adding additional things to the I didn't really like that so I went ahead yesterday and build #696 . I refactored your contribution here to fall into that new model. |
|
@edeandrea makes sense, the config split is way cleaner than the default-method workaround. Thanks for adding me to the contributors too! |
2 participants
Closes #664
Implements the design discussed in the issue.
What changed
DoclingApiBuilder.asyncExecutor(Executor): new builder method, implemented byDoclingServeClientBuilder. It's adefaultmethod throwingUnsupportedOperationException, so existing third-party builder implementations keep compiling.nullis rejected withIllegalArgumentException, like the other builder arguments.@Nullabledown toAsyncOperations, which uses it for every step of an async operation: submitting the task, each status poll, the delayed re-poll and retrieving the result. When no executor is set it uses the 1-argsupplyAsync/ 2-argdelayedExecutor, so the behaviour is exactly the previous one (CompletableFuture's default executor is not alwaysForkJoinPool.commonPool()).toBuilder()carries the executor over. The client never shuts it down – its lifecycle belongs to the caller.ConvertOperations,ChunkOperationsandAsyncOperationsgain constructor overloads taking the executor; their existing constructors are unchanged.Tests
AbstractDoclingServeClientAsyncExecutorTests, run for both clients byDoclingServeJackson2ClientAsyncExecutorTests/DoclingServeJackson3ClientAsyncExecutorTests(WireMock), drives tasks through submit →startedpoll → delayed re-poll → final status (→ result) and counts the tasks submitted to the executor: 5 for successful convert and hybrid-chunk tasks, 4 for a failed task. It also covers the default executor,toBuilder()andnull.DoclingServeApiTests.asyncExecutorIsUnsupportedByDefaultcovers the default method../gradlew spotlessCheckand the new tests pass locally.Note:
spotlessApplyre-formatted a few pre-existing lines in the touched files (DoclingServeApi,DoclingServeClient,AsyncOperations,ConvertOperations), which is why the diff is a bit larger than the change itself. Happy to squash the review commits once this is approved.