Skip to content

Fix OOM when updating very large metadata datasources - #390

Open
adimiz1 wants to merge 2 commits into
masterfrom
fix/datasource-update-oom
Open

adimiz1 wants to merge 2 commits into
masterfrom
fix/datasource-update-oom

Conversation

@adimiz1

@adimiz1 adimiz1 commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Updating a very large metadata datasource (~100k+ entries, e.g. Best Buy's upc field, SNI-8550) with updateMetadataFieldDatasource caused OutOfMemoryError. The API echoes the whole datasource in the response, and the SDK held it in memory several times over: body String -> JSONObject tree -> Map tree. A plain HTTP client that ignores the body does not have this problem.

Changes

  • New opt-in option skip_response_body: on a 200 the body is drained, not parsed, and an empty response is returned. Status and rate-limit headers are still available, and errors still throw as before.
  • New overloads Api.updateMetadataFieldDatasource(id, entries, options) and Api.restoreDatasourceEntries(id, ids, options). The existing signatures are unchanged and delegate to them.
  • Successful responses are parsed straight from the stream (explicit UTF-8) instead of being buffered as a String first, and the response is now closed.
  • Network failures while reading the body still raise GeneralError, as before.
  • A non-200 response without an error key now maps to the proper exception instead of an NPE.
  • Request side is unchanged (streaming it would make the entity non-repeatable and chunked).

Compatibility

No public signature changes. The only behavior difference is the non-200-without-error-key case above. The default path still builds a full object tree, so very large datasources should use skip_response_body. A backend no_entries_in_response param would be the full fix and is out of scope.

Tests

  • New DatasourceLargeResponseTest (offline, local JDK HttpServer): large response, large request, skip option, UTF-8 round trip, error mapping, missing error key, malformed JSON, empty body, non-JSON error body, dropped connection mid-body.
  • The test uses 300k entries by default (override with -Ddatasource.test.entries). Run under a small heap (e.g. -Xmx256m): on the old code 3 tests fail, including the OOM; on this branch all pass.
  • Full http5 live suite passes (0 failures).

🤖 Generated with Claude Code

adimiz1 and others added 2 commits October 18, 2026 11:07
… OOM

Updating a very large metadata datasource (~100k+ entries) caused
OutOfMemoryError because the response, which echoes the whole datasource,
was buffered as a String and then materialized as two object trees.

- Add opt-in `skip_response_body` option: the body is drained, not parsed,
  and an empty response is returned (status and rate-limit headers kept).
- Add `updateMetadataFieldDatasource` and `restoreDatasourceEntries`
  overloads that accept options. Existing signatures are unchanged.
- Parse successful responses directly from the stream (UTF-8) instead of
  buffering a String, and close the response.
- Network failures while reading the body still raise GeneralError.
- A non-200 response without an `error` key now maps to the proper
  exception instead of an NPE.
- Add offline tests against a local HTTP server and a `memoryTest` Gradle
  task running them under a 256m heap.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@const-cloudinary const-cloudinary left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend addressing these before merging:

  1. Consider adding deleteDatasourceEntries(id, ids, options). The delete endpoint also returns the full datasource (Admin API: Delete entries in a metadata field datasource), so large datasources would hit the same memory issue there. An overload matching the other two would cover it.
  2. Suggest aligning the @return javadoc on updateMetadataFieldDatasource and the response mock in DatasourceLargeResponseTest with the documented response, which is {"values":[...]} (Admin API: Update a metadata field datasource) rather than the field definition.
  3. Suggest checking for a null body explicitly in buildError (ApiStrategy.java:140) rather than catching NullPointerException alongside JSONException.

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.

2 participants