Skip to content

perf(indexing): soft-commit instead of hard-commit, keeping documents searchable - #196

Open
adityamparikh wants to merge 5 commits into
apache:mainfrom
adityamparikh:feat/indexing-batch-guidance
Open

adityamparikh wants to merge 5 commits into
apache:mainfrom
adityamparikh:feat/indexing-batch-guidance

Conversation

@adityamparikh

@adityamparikh adityamparikh commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Summary

Two small changes to inline indexing.

  • Payload guidance. Inline payloads are generated by the model as tool arguments, so every byte costs output tokens and one oversized call is truncated at the model's output limit. The server instructions (spring.ai.mcp.server.instructions) now tell the model: send data in the format it already has; when converting (for example from a paste), emit CSV and never convert to XML; pass JSON as the documents array, not a string; split inputs larger than a few hundred KB across several calls, not further than needed because each call commits. These are directives rather than a cost comparison, because a live run showed the model ignoring the comparison and emitting escaped JSON. Instructions are sent once per session, so this is the one home for it; tool descriptions are unchanged.
  • Soft commit per call. Splitting multiplies commits, so indexDocuments now ends with a soft commit (waitFlush=false, waitSearcher=true, softCommit=true) instead of a hard commit. Documents are still searchable when the tool returns; the segment fsync is left to Solr's autoCommit, which the _default configset enables at 15 s, and the transaction log covers durability in between. README notes that a configset with autoCommit disabled should enable it.

Tests

  • IndexingServiceTest verifies the soft-commit overload is used and the hard-commit overload is not.
  • New McpServerInstructionsTest pins the splitting clause and the convert-to-CSV directive in application.properties.

Verification

./gradlew build on Java 25: green (406 tests, 0 failures, 7 skipped at the previous head; unchanged test set plus one assertion) (the OTLP suite, skipped on main until #198). Independent of #197: the only shared file is IndexingServiceTest, and neither branch stubs commit there.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CiUHyyXLTo9ATdgg8eRFZJ

adityamparikh added a commit to adityamparikh/solr-mcp that referenced this pull request Sep 14, 2026
The mock returns null without it, and a strict stub on the one-argument
commit would be flagged unnecessary once apache#196's soft commit lands.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CiUHyyXLTo9ATdgg8eRFZJ
Signed-off-by: Aditya Parikh <aditya.m.parikh@gmail.com>
adityamparikh and others added 4 commits September 14, 2026 11:55
Solr already parses CSV and its own update XML. The server's CSV and XML
document creators re-implemented that, and the XML one did it with a
convention of its own: a generic <shows><show> mapping that prefixed
every field with the record element (show_title) and dropped the id, so
XML was the one format whose documents did not match the same data in
JSON, CSV or Markdown.

Both creators are removed, along with the orchestrator's CSV/XML paths
and the commons-csv dependency. The two tools now forward the payload,
as given, to Solr's /update handler:

- index-csv-documents sends the CSV with header=true. Solr reads the
  header for the field names, so column names are used as given.
  Repeated column names are multi-valued fields and empty cells are
  skipped.
- index-xml-documents takes Solr update XML, <add><doc><field
  name="...">, the format every Solr user already has.

Solr's update XML grammar is a command language: <delete>, <commit>,
<optimize> and <rollback> go to the same endpoint as <add>, so a tool
that forwarded blindly would let an indexing call delete a collection.
SolrUpdateXml.requireAddBlock reads the payload with a hardened StAX
parser (DTD off, external entities off) only as far as the root element
and rejects a DOCTYPE or any root but <add>; Solr parses the rest.

Solr's update response carries a status and a QTime but no document
count, so the two tools report that Solr accepted and committed the
payload instead of an invented "indexed N of N", and the index-data
prompt points at the health check for the count.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MJSxr89SRAa7BTC8Jg27Rj
Signed-off-by: Aditya Parikh <aditya.m.parikh@gmail.com>
…egration tests

Port shows.csv, shows.xml, and ShowsSampleDataIntegrationTest from PR apache#201.
The integration test verifies that JSON, CSV, and XML datasets index the
exact same 61 documents into Solr.

Signed-off-by: Aditya Parikh <aditya.m.parikh@gmail.com>
Co-authored-by: Junie <junie@jetbrains.com>
…nd trip

Follow-up cleanup on the CSV/XML pass-through, no behaviour change beyond
the commit folding below.

Reuse:
- The XML content type was spelled out as a literal; SolrJ ships it as
  ClientUtils.TEXT_XML with exactly that value. (The CSV side keeps its
  literal: ContentStreamBase.TEXT_CSV is private.)
- SolrUpdateXml configured an XMLInputFactory by hand. Spring's
  StaxUtils.createDefensiveInputFactory() sets the same two properties and
  additionally installs a no-op XMLResolver, so it is strictly more
  hardened. It is now a static field: XMLInputFactory.newFactory() runs a
  ServiceLoader scan of the whole classpath on every call, and the factory
  is never reconfigured after construction, which is the sharing contract
  StAX requires.
- IndexingServiceIntegrationTest is a @SpringBootTest that overwrote its
  own @Autowired beans with hand-built ones, under a comment claiming it
  is not a Spring Boot test. IndexingService is @observed, so the test was
  exercising an unproxied object rather than the one the application runs.

Efficiency:
- forward() posted the payload and then posted a separate commit. The
  commit now rides on the same request via setAction(ACTION.COMMIT), which
  removes a round trip per CSV/XML call and makes the status and QTime the
  message reports actually cover the commit it claims. The two mock tests
  that pinned the second call now assert commit=true on the request.

Simplification:
- describeIndexedFields/describeFieldNames was split when three tools
  reported field names; only the JSON tool does now, so it is one method
  again. The indexDocuments javadoc that had drifted onto a constant is
  reattached.
- SolrUpdateXml's ClosingReader record existed only to make one
  XMLStreamReader try-with-resources-able; the source is an in-memory
  StringReader and XMLStreamReader.close() does not close it.
- Removed the emptied "Apache Commons" heading left by dropping
  commons-csv.

Docs that still described the deleted parsers: IndexingDocumentCreator's
class javadoc, the test tree in dev-docs/ARCHITECTURE.md, and the FAQ's
claim that CSV and XML get field sanitization and 10 MB guards.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014XuwC2kdJ6Q1dDDZMHDCee
Signed-off-by: Aditya Parikh <aditya.m.parikh@gmail.com>
The sample data was made readable from the native test binary with a raw
hosted option in the Gradle args list:

    -H:IncludeResources=shows\.(csv|xml)$

That is the wrong seam and the wrong scope. dev-docs/graalvm-native-image.md
says to add a targeted registration through a hints registrar and prefer it
over other mechanisms "so the rule is explicit and reviewed", so the next
person adding a fixture does not find the precedent where the docs point.
And the rule being encoded is not the name of one dataset.

Replaced with a TestRuntimeHintsRegistrar wired through
src/test/resources/META-INF/spring/aot.factories, which spring-test invokes
for every test class. It lives in test sources deliberately: SolrNativeHints
is the production equivalent and must not ship test fixture names in the
application image.

The patterns name the three fixtures rather than matching by extension.
registerPattern compiles * to .*, which crosses /, so a *.xml here would
embed all 124 XML resources present on this project's 210-jar test
classpath, declaring every dependency's XML to be a test fixture.

Registering shows.json alongside its siblings is not redundant: Spring AOT
registers .*\.json globally, so the JSON fixture was covered by the
framework while the CSV and XML ones were not. Covering all three in one
place makes the dataset intentional rather than two-thirds accidental.

Verified with ./gradlew nativeTest -Pnative: 244 passed, 0 failed, 142
skipped (skips unchanged from baseline), with
ShowsSampleDataIntegrationTest executing natively. A negative control with
the registrar unregistered fails that test alone, on
"NullPointerException: missing test resource /shows.csv", confirming the
registration is load-bearing and not a no-op replacing an unnecessary flag.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014XuwC2kdJ6Q1dDDZMHDCee
Signed-off-by: Aditya Parikh <aditya.m.parikh@gmail.com>
@adityamparikh
adityamparikh force-pushed the feat/indexing-batch-guidance branch from 95fca5b to 4b0815f Compare September 15, 2026 16:57
@adityamparikh

Copy link
Copy Markdown
Contributor Author

Rebased onto #205 and pushed. This PR now depends on #205 and should merge after it. The first four commits shown are #205's; GitHub cannot retarget the base to a branch on the fork, so they appear here until #205 lands.

The rebase was necessary rather than cosmetic. #205 introduces forward(), the CSV/XML path. Before the rebase this PR could not reach it, so a caller that splits a large input across several calls does not pay for a hard commit per call was only true for JSON and markdown; CSV and XML still issued setAction(COMMIT, waitFlush=true, waitSearcher=true). Neither PR is wrong alone — the gap only exists once both are merged, which is exactly why it survived review twice.

Three fixes on top:

Soft commit for CSV and XML (a4fb3d3). Measured against Solr directly, same endpoint and document, median of 10: hard commit 22.79 ms vs soft 8.26 ms. Across the MCP tools, 61 single-document CSV calls go from 1853 ms to 585 ms (per extra call 30.4 ms to 9.5 ms). JSON and markdown, which this did not touch, stayed flat — that control is what makes the number trustworthy rather than a warm-cache artifact.

The size threshold was wrong, and unreachable (205a32d). Observed live: Sonnet 5 in Claude Desktop was asked to index an 86 KB markdown file. After three minutes nothing had reached the server — the model was still emitting the payload as tool arguments, about 76,000 output tokens, past the output budget, so the call could only ever truncate and fail. The instructions said to split inputs larger than a few hundred KB; for record data one byte costs roughly one output token, so that threshold is multiples past any output budget. Now ~20 KB per call, with the byte-per-token rule stated so a caller can size its own input, and an instruction to count and batch before writing the first call. The guidance also moved into the tool descriptions: instructions is returned once at initialize and not every client puts it in the model's context, whereas tool descriptions always are. That reverses 6f15281 deliberately — happy to drop it if you would rather keep the single source.

Input format vs conversion target (4b0815f). Send data in the format you already have; when you must convert, emit CSV, and never convert to XML put those two clauses side by side, so a model holding XML could read never XML as applying to its input — and converting XML is the worst possible move, since #205 forwards it to Solr unparsed. Now stated separately, with conversion scoped to formats that have no tool of their own. Only index-markdown-documents previously carried an as-is rule; all four do now.

Build green: 392 tests, 0 failures, 7 skipped (the pre-existing @Disabled OTLP class).

… searchable

Every indexing tool ended its work with a hard commit: indexDocuments called
solrClient.commit(collection), and the CSV/XML path added by apache#205 issued
setAction(COMMIT, waitFlush=true, waitSearcher=true). A hard commit fsyncs the
segments, so each tool call waited on the storage device.

That is not what Solr's own defaults do. The _default configset ships
autoCommit at maxTime 15000 with openSearcher=false, and autoSoftCommit at
3000: a background hard commit purely to truncate the transaction log, and soft
commits for visibility. Forcing a synchronous fsync per tool call fought that
design.

Both paths now commit with waitFlush=false, waitSearcher=true, softCommit=true.
waitSearcher keeps the guarantee that matters to a tool caller: the documents
are searchable the moment the call returns. Verified 30/30 with zero delay
across the JSON and CSV paths.

Durability is unchanged. The transaction log is written on the add, before any
commit, so documents survive a crash regardless of commit mode; a hard commit
governs how much tlog must be replayed on recovery, not whether data is lost.
That housekeeping stays with autoCommit.

Measured against Solr, 20 interleaved reps, same endpoint and document, only
the commit parameter varying:

  no commit      4.05 ms median
  soft commit    8.61 ms median, p90 10.65
  hard commit   18.94 ms median, p90 41.32

2.2x faster and far tighter -- the hard commit's p90 is four times its median,
which is fsync variance. Over the MCP tools, 61 single-document CSV calls go
from 1853 ms to 585 ms.

Operators running a custom configset with autoCommit disabled should enable it,
or the transaction log grows until something else commits.

Signed-off-by: Aditya Parikh <aditya.m.parikh@gmail.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@adityamparikh
adityamparikh force-pushed the feat/indexing-batch-guidance branch from 4b0815f to 44ea747 Compare September 15, 2026 18:34
@adityamparikh adityamparikh changed the title feat(indexing): advise payload splitting and soft-commit per call perf(indexing): soft-commit instead of hard-commit, keeping documents searchable Sep 15, 2026
@adityamparikh

Copy link
Copy Markdown
Contributor Author

Reduced to one commit. All payload-splitting and format guidance is reverted — the instructions, the tool descriptions and the index-data prompt are untouched by this PR. What remains is the commit-mode change, verified against the condition that documents stay searchable.

Why the guidance went away. It did not work, and the MCP maintainers consider the shape of it an anti-pattern. Tested live: Sonnet 5 in Claude Desktop, an 86 KB markdown paste. With the guidance in place the model did batch correctly — 15 calls of 10 documents, no truncation — and still took over 15 minutes, because splitting does not reduce total output tokens and each extra call re-reads the whole conversation. Meanwhile discussion #1197 records the consensus as keep the data plane out of JSON-RPC arguments entirely, naming "passing entire file content through LLM context" as an anti-pattern, and the File Uploads WG charter names "prose instructions asking for base64 strings or local paths" as the problem it exists to remove. Tuning prose was treating a symptom the spec intends to delete; the real answer is the handle-based path (SEP-2356, SEP-2631), which is bigger than this PR.

Why the commit change stays. Both paths hard-committed — indexDocuments via solrClient.commit(collection), and #205's CSV/XML forward() via setAction(COMMIT, waitFlush=true, waitSearcher=true) — so every tool call waited on an fsync. Solr's own _default configset does the opposite: autoCommit at 15 s with openSearcher=false for tlog truncation, autoSoftCommit at 3 s for visibility. We were fighting the configset.

Measured, 20 interleaved reps, same endpoint and document, only the commit parameter varying:

mode median p90
no commit 4.05 ms 4.64 ms
soft commit 8.61 ms 10.65 ms
hard commit 18.94 ms 41.32 ms

2.2x faster, and far tighter — the hard commit's p90 is four times its median, which is fsync variance. Over the MCP tools, 61 single-document CSV calls go from 1853 ms to 585 ms.

Searchability is preserved, which was the condition: waitSearcher=true means documents are visible when the call returns, verified 30/30 with zero delay on both the JSON and CSV paths. Durability is unchanged — the transaction log is written on the add, before any commit, so documents survive a crash in every commit mode; a hard commit governs how much tlog is replayed on recovery, not whether data is lost. One caveat for the docs: an operator running a custom configset with autoCommit disabled should enable it.

Still depends on #205 and should merge after it. Build green: 390 tests, 0 failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant