TIKA-4839 - simplify signature - #3081
Merged
Merged
Conversation
tballison
force-pushed
the
TIKA-4839-tweaks
branch
from
August 27, 2026 11:39
a096a09 to
785dccc
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Simplifies the ServerManager restart-reporting API to a single generation-aware spelling and tightens client/manager behavior around worker restarts and shutdown races.
Changes:
- Remove legacy
ServerManagerrestart-reporting overloads in favor ofmarkServerForRestart(RestartReason, long)andhandleCrashAndGetExitCode(long)as abstract methods. - Update server-manager implementations and test fakes to track
generationand restart marking consistently. - Make
PipesClientgracefully returnFAILED_TO_INITIALIZEwhen initialization races with manager shutdown.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tika-pipes/tika-pipes-integration-tests/src/test/java/org/apache/tika/pipes/core/PipesClientTest.java | Adds an assertion that the client backstop triggers a worker restart mark. |
| tika-pipes/tika-pipes-core/src/test/java/org/apache/tika/pipes/core/ServerManagerMarkContractTest.java | Removes now-obsolete tests for deprecated overload spellings. |
| tika-pipes/tika-pipes-core/src/test/java/org/apache/tika/pipes/core/SentinelServerManager.java | Updates test sentinel to implement new abstract ServerManager methods and capture restart reason. |
| tika-pipes/tika-pipes-core/src/test/java/org/apache/tika/pipes/core/PipesClientPayloadLimitTest.java | Asserts oversized payload rejection does not recycle a healthy worker. |
| tika-pipes/tika-pipes-core/src/test/java/org/apache/tika/pipes/core/PipesClientInterruptTest.java | Asserts interrupt recycling happens via abandonment, not explicit marking (avoids double counting). |
| tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/SharedServerManager.java | Removes old overloads; relies on the new generation-aware methods. |
| tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/ServerManager.java | Collapses restart-reporting surface to one spelling; makes methods abstract. |
| tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/PipesClient.java | Converts an init-time IllegalStateException into a FAILED_TO_INITIALIZE result. |
| tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/PerClientServerManager.java | Adds per-client generation tracking; synchronizes restart marking; implements new API. |
| CHANGES.txt | Documents the behavior and API surface changes under TIKA-4839. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…IALIZE with a test
| } | ||
|
|
||
| @Override | ||
| public void close() { |
Comment on lines
+238
to
+247
| } catch (IllegalStateException e) { | ||
| // The manager was closed underneath us: a request thread racing PipesParser.close() | ||
| // or AsyncProcessor.close(), which interrupts workers without awaiting them. Nothing | ||
| // to restart and nothing to recover -- but report it rather than letting an unchecked | ||
| // exception escape PipesParser.parse() to a caller that cannot act on it. | ||
| LOG.warn("clientId={}: server manager closed while initializing {}", pipesClientId, | ||
| t.getId(), e); | ||
| closeConnection(); | ||
| return buildFatalResult(t.getId(), t.getEmitKey(), PipesResult.RESULT_STATUS.FAILED_TO_INITIALIZE, | ||
| intermediateResult.get(), e.getMessage()); |
Comment on lines
+104
to
120
| long getGeneration(); | ||
|
|
||
| /** | ||
| * The reasonless spelling of the above, kept for callers that cannot attribute the failure. | ||
| * Routed through the reason form rather than the bare no-arg default: that default exists | ||
| * only to keep pre-RestartReason implementations working, and delegating here would leave | ||
| * this silently inert for any implementation that overrides only the reason form. | ||
| */ | ||
| default void markServerForRestart(long generation) { | ||
| markServerForRestart(RestartReason.CRASH, generation); | ||
| } | ||
|
|
||
| /** | ||
| * As {@link #handleCrashAndGetExitCode()}, but only if {@code generation} is still current. | ||
| * Marks the server for restart due to a fatal error, attributed to {@code reason}, but only | ||
| * if {@code generation} is still current -- reports about a superseded process are dropped. | ||
| * <p> | ||
| * Called by a client that received a fatal status: the process is stopping even if | ||
| * {@link #isRunning()} still says otherwise, and the next {@link #ensureRunning()} waits for | ||
| * it to exit and restarts it. | ||
| * <p> | ||
| * Deliberately the only spelling, and deliberately abstract. Earlier revisions offered a | ||
| * no-arg and a reasonless form defaulting to one another; an implementation that overrode | ||
| * only one left the others silently inert, which is how a worker known to be poisoned kept | ||
| * being handed documents. | ||
| */ | ||
| default int handleCrashAndGetExitCode(long generation) { | ||
| return handleCrashAndGetExitCode(); | ||
| } | ||
| void markServerForRestart(RestartReason reason, long generation); | ||
|
|
Comment on lines
3
to
12
| * tika-pipes: a parse that raced PipesParser.close()/AsyncProcessor.close() | ||
| threw IllegalStateException out of PipesParser.parse() in per-client mode; | ||
| both modes now return a FAILED_TO_INITIALIZE result, which a caller can act | ||
| on. ServerManager's restart-reporting surface is reduced to one spelling -- | ||
| markServerForRestart(RestartReason, long) and handleCrashAndGetExitCode(long), | ||
| both abstract. The previous no-arg and reasonless forms defaulted to one | ||
| another, so an implementation that overrode only one left the others | ||
| silently inert (TIKA-4839). | ||
|
|
||
| * Add Micrometer reporting and opt-in endpoint for tika-server (TIKA-4839). |
…ets closed, name unused param
Contributor
Author
|
Last commit was vanity to say something to copilot. I think we're good. |
Comment on lines
+104
to
+119
| long getGeneration(); | ||
|
|
||
| /** | ||
| * The reasonless spelling of the above, kept for callers that cannot attribute the failure. | ||
| * Routed through the reason form rather than the bare no-arg default: that default exists | ||
| * only to keep pre-RestartReason implementations working, and delegating here would leave | ||
| * this silently inert for any implementation that overrides only the reason form. | ||
| */ | ||
| default void markServerForRestart(long generation) { | ||
| markServerForRestart(RestartReason.CRASH, generation); | ||
| } | ||
|
|
||
| /** | ||
| * As {@link #handleCrashAndGetExitCode()}, but only if {@code generation} is still current. | ||
| * Marks the server for restart due to a fatal error, attributed to {@code reason}, but only | ||
| * if {@code generation} is still current -- reports about a superseded process are dropped. | ||
| * <p> | ||
| * Called by a client that received a fatal status: the process is stopping even if | ||
| * {@link #isRunning()} still says otherwise, and the next {@link #ensureRunning()} waits for | ||
| * it to exit and restarts it. | ||
| * <p> | ||
| * Deliberately the only spelling, and deliberately abstract. Earlier revisions offered a | ||
| * no-arg and a reasonless form defaulting to one another; an implementation that overrode | ||
| * only one left the others silently inert, which is how a worker known to be poisoned kept | ||
| * being handed documents. | ||
| */ | ||
| default int handleCrashAndGetExitCode(long generation) { | ||
| return handleCrashAndGetExitCode(); | ||
| } | ||
| void markServerForRestart(RestartReason reason, long generation); |
| markServerForRestart(RestartReason.CRASH); | ||
| return -1; | ||
| } | ||
| int handleCrashAndGetExitCode(long generation); |
Comment on lines
+1
to
+5
| Release 4.1.0 - unreleased | ||
|
|
||
| * PDF: extractFontNames threw NullPointerException on a page with no | ||
| /Resources dictionary (TIKA-4842). | ||
|
|
||
| * tika-server: opt-in Micrometer metrics reporting and endpoint | ||
| (TIKA-4839). | ||
|
|
||
| * Per-request (parse-context) config for parsers that lock fields | ||
| (Tess4J, VLM, OpenAI image-embedding) threw even when empty; locked | ||
| fields are still rejected when actually set (TIKA-4843). | ||
|
|
||
| * OOXML: new msoffice:has-unreferenced-parts and | ||
| msoffice:unreferenced-part-names flag package parts unreachable via the | ||
| OPC relationship graph. Structural only, expect false positives; not | ||
| applied to XPS (TIKA-4837). | ||
|
|
||
| * Shared pipes server (useSharedServer: true): a worker death could trigger | ||
| a second, spurious restart that killed the healthy replacement. Forks now | ||
| carry a generation; stale reports are dropped. Also fixed: a fork after | ||
| shutdown() that was never destroyed, and a temp-dir leak on interrupt | ||
| during teardown (TIKA-4844). | ||
|
|
||
| * tika-pipes cache memory budget defaults to a quarter of the fork heap; | ||
| override with -Dtika.pipes.cacheMemoryBudgetBytes in forkedJvmArgs | ||
| (<=0 disables). TikaInputStream: hasFile() also reports cache spills, | ||
| toString() no longer spills, new inMemoryContent(channel). Digester gains | ||
| digestSink(); a digest is published only on commit(), so failed or empty | ||
| translations (e.g. stub PST items) publish no digest. New | ||
| TemporaryResources.closeAll(Closeable...) (TIKA-4835). | ||
|
|
||
| * Docs/javadocs reconciled with the code: ES/OpenSearch attachmentStrategy | ||
| has no default; Kafka connectionsMaxIdleMs is honored; jdbc | ||
| queryTimeoutSeconds 0 is not "no limit"; Solr basic auth only; pipes | ||
| reporters/iterators are built at config load; Tess4J also locks poolSize | ||
| and maxImagePixels; pdf:trapped is new, not renamed; plugin config | ||
| nesting fixed in 23 javadocs (TIKA-4842). | ||
|
|
||
| * Pipes plugins no longer bundle Jackson; the host provides it. Plugin | ||
| config is parsed by a shared strict PluginJson mapper (rejects unknown | ||
| and duplicate keys; accepts comments) (TIKA-4840). | ||
|
|
||
| * tika-server and tika-async-cli accept // and /* */ comments in config | ||
| during override merging, as documented (TIKA-4834). | ||
|
|
||
| * Kafka pipes iterator no longer stops on the first empty poll; waits for | ||
| partition assignment (assignmentTimeoutMs, 30s) and a quiet window | ||
| (drainIdleMs, 1s). groupInitialRebalanceDelayMs is deprecated | ||
| (TIKA-4833). | ||
|
|
||
| * Pipes IPC carries inline bytes as a raw binary field, not in the tuple; | ||
| Smile 7-bit binary encoding disabled. 4.0.0 tuples with an "inline-bytes" | ||
| parse-context entry are rejected (TIKA-4829). | ||
|
|
||
| * Digesting embedded documents no longer spools each to a temp file; a | ||
| process-wide CacheMemoryBudget (default 256MB) keeps them in memory. New | ||
| TikaInputStream API: get(IOSupplier,...), enableRewind(CacheMemoryBudget), | ||
| getSeekableByteChannel(). Zip-family parsing and detection use seekable | ||
| channels, so hasFile() may be false afterward; getPath() still spools on | ||
| demand (TIKA-4828). | ||
|
|
||
| * Pipes carries the client Content-Type into the forked worker as a | ||
| detection hint for all forked endpoints; honored only when it equals or | ||
| specializes the detected type, or when there is no magic. The | ||
| user-override key is not carried (TIKA-4825). | ||
|
|
||
| * OneNote: document-order extraction, superseded revisions omitted, embedded | ||
| BLOBs extracted, warnings and relationship IDs in metadata, bounded | ||
| recursion/allocation; malformed files fall back to the legacy string dump | ||
| * tika-pipes: a parse racing PipesParser.close() now returns FAILED_TO_INITIALIZE | ||
| instead of throwing IllegalStateException. ServerManager restart reporting is | ||
| now the single abstract pair markServerForRestart(RestartReason, long) and |
| public void closedManagerDuringInitReturnsFailedToInitialize() throws Exception { | ||
| try (ServerSocket serverSocket = new ServerSocket(0)) { | ||
| SentinelServerManager manager = new SentinelServerManager(serverSocket.getLocalPort()); | ||
| manager.closed = true; |
Comment on lines
+238
to
248
| } catch (IllegalStateException e) { | ||
| // Typically the manager was closed underneath us: a request thread racing PipesParser.close() | ||
| // or AsyncProcessor.close(), which interrupts workers without awaiting them. Nothing | ||
| // to restart and nothing to recover -- but report it rather than letting an unchecked | ||
| // exception escape PipesParser.parse() to a caller that cannot act on it. | ||
| LOG.warn("clientId={}: server manager rejected initialization of {}", pipesClientId, | ||
| t.getId(), e); | ||
| closeConnection(); | ||
| return buildFatalResult(t.getId(), t.getEmitKey(), PipesResult.RESULT_STATUS.FAILED_TO_INITIALIZE, | ||
| intermediateResult.get(), e.getMessage()); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thanks for your contribution to Apache Tika! Your help is appreciated!
Before opening the pull request, please verify that
TIKA-XXXX)[TIKA-XXXX] Issue or pull request title)./mvnw clean testmainbranch. If there are conflicts, please try to rebase the pull request branch on top of a freshly pulledmainbranchtika-bom/pom.xml.We will be able to faster integrate your pull request if these conditions are met. If you have any questions how to fix your problem or about using Tika in general, please sign up for the Tika mailing list. Thanks!