Add batchStrategy to batch, and progressTrace, writeChannelCongestion, and internalDatabaseSizes to batches stats#936
Conversation
📝 WalkthroughWalkthroughThis pull request extends the batch statistics data model to support Meilisearch v1.14.0 features. It introduces three new model classes (EmbedderRequests, WriteChannelCongestion, InternalDatabaseSizes) and augments existing classes (Batch, StatDetails) with corresponding fields to expose detailed batch metrics including progress tracking and resource congestion data. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@src/main/java/com/meilisearch/sdk/model/batch/res/InternalDatabaseSizes.java`:
- Around line 1-11: Delete the brittle InternalDatabaseSizes class and instead
change the StatDetails model to use an untyped mapping for internalDatabaseSizes
(e.g. replace the InternalDatabaseSizes-typed field in class StatDetails with a
HashMap<String,String> named internalDatabaseSizes and add the necessary
import), ensuring consistency with the existing progressTrace handling; this
removes the incorrect int fields, avoids Lombok/@Data issues on
InternalDatabaseSizes, and preserves forward compatibility with changing API
keys.
In `@src/main/java/com/meilisearch/sdk/model/batch/res/StatDetails.java`:
- Around line 17-18: Update StatDetails to treat writeChannelCongestion and
internalDatabaseSizes like progressTrace by replacing their concrete types
WriteChannelCongestion and InternalDatabaseSizes with Map<String,Object> (e.g.,
change field types, getters/setters, constructor params and usages). Keep the
field names writeChannelCongestion and internalDatabaseSizes intact for JSON
mapping, add/import java.util.Map if needed, and ensure any
serialization/deserialization annotations or usages that expect the old types
are adjusted to accept a Map so the class remains resilient to upstream schema
changes.
In
`@src/main/java/com/meilisearch/sdk/model/batch/res/WriteChannelCongestion.java`:
- Around line 5-10: The WriteChannelCongestion class currently uses snake_case
fields and an int for blocking_ratio; update it by either (A) keeping a typed
POJO: rename fields to camelCase (attempts, blockingAttempts, blockingRatio),
change blockingRatio to double, and add JSON mapping (either annotate class with
`@JsonNaming`(PropertyNamingStrategies.SnakeCaseStrategy.class) or annotate fields
with `@JsonProperty`("blocking_attempts")/@JsonProperty("blocking_ratio")) to
preserve deserialization; or (B) follow the upstream guidance and replace the
typed class with an untyped representation (e.g., HashMap<String,Object>) as
recommended alongside InternalDatabaseSizes and issue `#842` to avoid brittle
typings for changing batch stats. Ensure references to WriteChannelCongestion in
code are updated accordingly.
In `@src/test/java/com/meilisearch/integration/BatchTest.java`:
- Line 43: The test in BatchTest currently asserts that progressTrace is
non-null (batch.getStats().getProgressTrace()) but progressTrace is optional and
this makes the test flaky; change the assertion to guard for null (e.g., only
assert contents when progressTrace != null) or select a batch returned by
getAllBatches() that is known to include a progress trace, and add an assertion
that batch.getStats().getBatchStrategy() is non-null (since batchStrategy should
be present on every batch response) to cover the new fields; locate the checks
around getAllBatches() and the Batch object (batch.getStats()) to implement the
conditional check and the new batchStrategy assertion.
🧹 Nitpick comments (1)
src/main/java/com/meilisearch/sdk/model/batch/res/StatDetails.java (1)
16-16: UseMap<String, String>instead ofHashMap<String, String>for consistency.The other map fields in this class (
status,types,indexUids) all use theMapinterface.progressTraceshould follow the same pattern.Proposed fix
- private HashMap<String, String> progressTrace; + private Map<String, String> progressTrace;This also allows removing the
java.util.HashMapimport.
| package com.meilisearch.sdk.model.batch.res; | ||
|
|
||
| public class InternalDatabaseSizes { | ||
| private int externalDocumentsId; | ||
| private int wordDocsId; | ||
| private int wordPairProximityId; | ||
| private int wordPositionDocsId; | ||
| private int wordFidDocIds; | ||
| private int fieldIdWordCountDocIds; | ||
| private String documents; | ||
| } |
There was a problem hiding this comment.
Critical: Missing @Data, wrong field types, and the class contradicts upstream guidance to leave this object untyped.
Multiple issues with this model:
-
Missing
@Dataannotation — without it, Lombok won't generate getters/setters and deserialization will fail silently (all fields remain default values). -
Wrong types — the Meilisearch API returns string values like
"944 KiB (+944 KiB)"and"9.11 MiB (+9.11 MiB)"for allinternalDatabaseSizesentries. Usingintwill cause deserialization errors. -
Incomplete and fragile field set — the API returns many more keys such as
main,word_prefix_docids,exact_word_prefix_docids,word_prefix_position_docids,word_prefix_fid_docids, which are missing here. The v1.14 release warns: "Please consider that the batches stats content can change anytime." -
Issue
#842explicitly advises against typing these objects because their internal fields may change in breaking ways. The correct approach is to use aHashMap<String, String>, consistent with howprogressTraceis already modeled inStatDetails.
Proposed fix: Replace with a type alias or use HashMap in StatDetails directly
Either remove this class entirely and change the field in StatDetails to:
private HashMap<String, String> internalDatabaseSizes;Or, if you prefer keeping a named type:
+import lombok.Data;
+import java.util.HashMap;
+
+@Data
-public class InternalDatabaseSizes {
- private int externalDocumentsId;
- private int wordDocsId;
- private int wordPairProximityId;
- private int wordPositionDocsId;
- private int wordFidDocIds;
- private int fieldIdWordCountDocIds;
- private String documents;
-}
+public class InternalDatabaseSizes extends HashMap<String, String> {
+}🤖 Prompt for AI Agents
In `@src/main/java/com/meilisearch/sdk/model/batch/res/InternalDatabaseSizes.java`
around lines 1 - 11, Delete the brittle InternalDatabaseSizes class and instead
change the StatDetails model to use an untyped mapping for internalDatabaseSizes
(e.g. replace the InternalDatabaseSizes-typed field in class StatDetails with a
HashMap<String,String> named internalDatabaseSizes and add the necessary
import), ensuring consistency with the existing progressTrace handling; this
removes the incorrect int fields, avoids Lombok/@Data issues on
InternalDatabaseSizes, and preserves forward compatibility with changing API
keys.
| private WriteChannelCongestion writeChannelCongestion; | ||
| private InternalDatabaseSizes internalDatabaseSizes; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inconsistent approach: progressTrace is correctly untyped, but writeChannelCongestion and internalDatabaseSizes use typed classes.
All three of these fields (progressTrace, writeChannelCongestion, internalDatabaseSizes) are subject to the same upstream warning that their internal structure can change at any time. progressTrace is modeled as a flexible map — the other two should follow the same pattern for consistency and resilience.
Consider changing both to Map<String, Object> to match the design intent from issue #842.
🤖 Prompt for AI Agents
In `@src/main/java/com/meilisearch/sdk/model/batch/res/StatDetails.java` around
lines 17 - 18, Update StatDetails to treat writeChannelCongestion and
internalDatabaseSizes like progressTrace by replacing their concrete types
WriteChannelCongestion and InternalDatabaseSizes with Map<String,Object> (e.g.,
change field types, getters/setters, constructor params and usages). Keep the
field names writeChannelCongestion and internalDatabaseSizes intact for JSON
mapping, add/import java.util.Map if needed, and ensure any
serialization/deserialization annotations or usages that expect the old types
are adjusted to accept a Map so the class remains resilient to upstream schema
changes.
| @Data | ||
| public class WriteChannelCongestion { | ||
| private int attempts; | ||
| private int blocking_attempts; | ||
| private int blocking_ratio; | ||
| } |
There was a problem hiding this comment.
blocking_ratio should be a floating-point type, and this class should ideally remain untyped per upstream guidance.
-
blocking_ratioasintis incorrect — a ratio is inherently a fractional value (e.g., 0.0–1.0). Usingintwill truncate or fail deserialization. Change todouble. -
Snake_case field names —
blocking_attemptsandblocking_ratioviolate Java naming conventions. Use camelCase with@JsonPropertyannotations for JSON mapping, or use@JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class)on the class. -
Same concern as
InternalDatabaseSizes— the v1.14 release warns "the batches stats content can change anytime," and writeChannelCongestion is described as "an object containing some internal information about the write congestion." Issue#842advises against typing these objects. Consider usingHashMap<String, Object>instead.
If keeping a typed class, at minimum fix the type and naming
package com.meilisearch.sdk.model.batch.res;
+import com.fasterxml.jackson.annotation.JsonProperty;
import lombok.Data;
`@Data`
public class WriteChannelCongestion {
private int attempts;
- private int blocking_attempts;
- private int blocking_ratio;
+ `@JsonProperty`("blocking_attempts")
+ private int blockingAttempts;
+ `@JsonProperty`("blocking_ratio")
+ private double blockingRatio;
}🤖 Prompt for AI Agents
In
`@src/main/java/com/meilisearch/sdk/model/batch/res/WriteChannelCongestion.java`
around lines 5 - 10, The WriteChannelCongestion class currently uses snake_case
fields and an int for blocking_ratio; update it by either (A) keeping a typed
POJO: rename fields to camelCase (attempts, blockingAttempts, blockingRatio),
change blockingRatio to double, and add JSON mapping (either annotate class with
`@JsonNaming`(PropertyNamingStrategies.SnakeCaseStrategy.class) or annotate fields
with `@JsonProperty`("blocking_attempts")/@JsonProperty("blocking_ratio")) to
preserve deserialization; or (B) follow the upstream guidance and replace the
typed class with an untyped representation (e.g., HashMap<String,Object>) as
recommended alongside InternalDatabaseSizes and issue `#842` to avoid brittle
typings for changing batch stats. Ensure references to WriteChannelCongestion in
code are updated accordingly.
| assertNotNull(batch.getStats().getStatus()); | ||
| assertNotNull(batch.getStats().getTypes()); | ||
| assertNotNull(batch.getStats().getIndexUids()); | ||
| assertNotNull(batch.getStats().getProgressTrace()); |
There was a problem hiding this comment.
progressTrace may be null for some batch types — this assertion could be flaky.
The Meilisearch v1.14 release notes warn that "the batches stats content can change anytime." The progressTrace field is optional in the API response, so the first batch returned by getAllBatches may not always include it. Consider guarding this with a conditional or testing with a batch known to have a progress trace.
Also, there's no test coverage for the other new fields (batchStrategy, writeChannelCongestion, internalDatabaseSizes, embedderRequests). At minimum, asserting batchStrategy is non-null would be valuable since it appears on every batch response.
🤖 Prompt for AI Agents
In `@src/test/java/com/meilisearch/integration/BatchTest.java` at line 43, The
test in BatchTest currently asserts that progressTrace is non-null
(batch.getStats().getProgressTrace()) but progressTrace is optional and this
makes the test flaky; change the assertion to guard for null (e.g., only assert
contents when progressTrace != null) or select a batch returned by
getAllBatches() that is known to include a progress trace, and add an assertion
that batch.getStats().getBatchStrategy() is non-null (since batchStrategy should
be present on every batch response) to cover the new fields; locate the checks
around getAllBatches() and the Batch object (batch.getStats()) to implement the
conditional check and the new batchStrategy assertion.
There was a problem hiding this comment.
What would be a good testing strategy for covering writeChannelCongestion, internalDatabaseSizes, embedderRequests test
valkrypton
changed the title
valkrypton
force-pushed
the
update/batch-stats
branch
from
8377435 to
a206163
Compare
1 participant
Pull Request
Related issue
Fixes #842 #872
What does this PR do?
batchStrategyto theBatchclassprogressTrace,writeChannelCongestionandinternalDatabaseSizetoStatsDetailclass.PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit