Skip to content

Use binary serde for resource usage headers - #21230

Merged
cwperks merged 5 commits into
opensearch-project:mainfrom
dzane17:binary-resource-usage-header
Jul 8, 2026
Merged

Use binary serde for resource usage headers#21230
cwperks merged 5 commits into
opensearch-project:mainfrom
dzane17:binary-resource-usage-header

Conversation

@dzane17

@dzane17 dzane17 commented Apr 15, 2026

Copy link
Copy Markdown
Member

Description

Replaces JSON serialization/deserialization of TaskResourceInfo in the TASK_RESOURCE_USAGE response header with Base64-encoded binary using the existing Writeable interface. This reduces CPU overhead on the search path where the resource usage header is parsed for every shard.

Problem

The TASK_RESOURCE_USAGE header carries per-shard resource usage data (CPU, memory) from data nodes to the coordinator. The current implementation serializes via toXContent (JSON) and deserializes via XContentParser (Jackson) on every shard response. Since a single search request can hit thousands of shards, this serde cost is multiplied accordingly. Profiling has shown ~7% CPU overhead from this serde.

Solution

Serialize TaskResourceInfo using writeTo/readFromStream (binary), Base64-encode the result into a string. Base64 encoding is necessary because ThreadContext response headers only support string values. Even if ThreadContext supported raw byte[] headers, benchmarks show the additional speedup is marginal (see data below), making the Base64 approach a good tradeoff that avoids modifying the public ThreadContext class.

Backwards compatibility

This change is fully BWC across a rolling upgrade.

  • Data nodes serialize with binary only when the receiving coordinator is on V_3_7_0 or later (resolved from the parent task ID via cluster state), otherwise serialize using the original JSON format.
  • On the deserialization path, coordinators try binary first, then fall back to JSON so they can read headers from new and old data nodes. This try/catch fallback is acceptable because the majority of the time the domain will not be mid-upgrade.

Rolling upgrade verification: Verified end-to-end with a 3-node cluster running released 3.6.1, upgrading one node at a time to this PR (3.7.0). Search workload driven against all three nodes at every phase: 852 searches total, 0 failures, no hangs, no stuck tasks, no parse errors on any node.

Benchmark Results (JMH, per 1000 shards)

The end-to-end round-trip (serialize on data node + deserialize on coordinator) is 5.9x faster with Binary+Base64 compared to JSON. Raw binary column is included to show the theoretical outcome if ThreadContext supported byte[] headers natively.

Operation JSON Binary+Base64 Raw Binary JSON→Base64 JSON→Raw
Serialize 227 μs 95 μs 81 μs 2.4x 2.8x
Deserialize 785 μs 220 μs 139 μs 3.6x 5.6x
Round-trip 1,647 μs 279 μs 252 μs 5.9x 6.5x

Each test result ran:

  • 3 warmup iterations
  • 3 measurement iterations

One iteration means JMH ran the target operation continuously for 10 seconds, then divided the total time by the number of completed operations to get a time per operation. Each operation performs 1000 TaskResourceInfo serde cycles — one per shard — to simulate a large search request hitting 1000 shards. The final values in the table are the average ns/op across all 3 measurement iterations.

Header Size Reduction

Based on a typical TaskResourceInfo:

{
  "action": "indices:data/read/search[phase/query]",
  "taskId": 1234567,
  "parentTaskId": 1234566,
  "nodeId": "U0rMsZg9RGOxdnVfMtMNVA",
  "taskResourceUsage": {
    "cpuTimeInNanos": 15000000,
    "memoryInBytes": 2048000
  }
}
Format Size
JSON ~196 bytes
Binary+Base64 ~136 bytes

Binary+Base64 reduces bytes sent over the wire by ~31% compared to JSON.

Related Issues

Resolves #17407

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@github-actions

github-actions Bot commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 2e2537a)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Version Gate Mismatch

The PR description states data nodes serialize binary only when the coordinator is on V_3_7_0 or later, but the code sets BINARY_RESOURCE_USAGE_HEADER_VERSION = Version.V_3_8_0. If the intent was V_3_7_0 (as documented and tested against a 3.6.1 rolling upgrade), the current constant will unnecessarily suppress the optimization for 3.7.x coordinators. Confirm which version this optimization actually shipped in and align the constant with the documented behavior.

static final Version BINARY_RESOURCE_USAGE_HEADER_VERSION = Version.V_3_8_0;

@github-actions

github-actions Bot commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 2e2537a

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Disambiguate header format before decoding

The consumer attempts binary decode first, but a legacy JSON header (starting with
{) will pass Base64 decoding for many payloads only to fail deep in stream parsing
(or worse — silently produce garbage before an exception). It's safer and cheaper to
sniff the first non-whitespace character: if it's {, go straight to the JSON path;
otherwise try binary. This also avoids masking real binary-parse bugs behind the
JSON fallback.

server/src/main/java/org/opensearch/tasks/TaskResourceTrackingService.java [420-429]

 try {
+    if (usage.charAt(0) == '{') {
+        return deserializeFromJson(usage);
+    }
     return WriteableBase64.decode(usage, TaskResourceInfo::readFromStream);
 } catch (Exception binaryFailure) {
     try {
         return deserializeFromJson(usage);
     } catch (Exception jsonFailure) {
         logger.debug("failed to parse task resource usage header (binary and JSON), skipping: ", jsonFailure);
         return null;
     }
 }
Suggestion importance[1-10]: 5

__

Why: Sniffing the leading { character to route JSON directly avoids potentially expensive/misleading binary decode attempts on legacy JSON payloads. It's a reasonable robustness improvement, though the existing fallback path already handles the case functionally.

Low
General
Detect trailing bytes after decoding

Base64.getDecoder() throws IllegalArgumentException on invalid input but will also
accept unexpected inputs (like accidental JSON payloads that happen to be valid
Base64 alphabet). Additionally, after reader.read(in) completes, the code doesn't
verify all bytes were consumed — trailing garbage will be silently ignored, which
can hide corruption or version-mismatch bugs. Consider asserting in.available() == 0
after reading.

server/src/main/java/org/opensearch/common/io/stream/WriteableBase64.java [59-64]

 public static <T> T decode(String encoded, Writeable.Reader<T> reader) throws IOException {
     byte[] bytes = Base64.getDecoder().decode(encoded);
     try (StreamInput in = StreamInput.wrap(bytes)) {
-        return reader.read(in);
+        T result = reader.read(in);
+        if (in.available() > 0) {
+            throw new IOException("Unexpected trailing bytes after decoding: " + in.available());
+        }
+        return result;
     }
 }
Suggestion importance[1-10]: 4

__

Why: Asserting no trailing bytes after decoding is a good defensive practice to catch corruption or version mismatches early, but it's a minor robustness improvement rather than a critical fix.

Low

Previous suggestions

Suggestions up to commit f416371
CategorySuggestion                                                                                                                                    Impact
General
Avoid exception-driven format dispatch

Checking the binary path first means every legacy JSON header (which starts with {)
incurs a Base64 decode attempt plus an exception before falling back. Since JSON
always starts with { and valid Base64 never does, cheaply dispatch on the first
character to avoid throw/catch on the hot path during rolling upgrades.

server/src/main/java/org/opensearch/tasks/TaskResourceTrackingService.java [411-430]

 public TaskResourceInfo getTaskResourceUsageFromThreadContext() {
     List<String> taskResourceUsages = threadPool.getThreadContext().getResponseHeaders().get(TASK_RESOURCE_USAGE);
     if (taskResourceUsages == null || taskResourceUsages.isEmpty()) {
         return null;
     }
     String usage = taskResourceUsages.get(0);
     if (usage == null || usage.isEmpty()) {
         return null;
     }
     try {
+        if (usage.charAt(0) == '{') {
+            return deserializeFromJson(usage);
+        }
         return WriteableBase64.decode(usage, TaskResourceInfo::readFromStream);
-    } catch (Exception binaryFailure) {
-        try {
-            return deserializeFromJson(usage);
-        } catch (Exception jsonFailure) {
-            logger.debug("failed to parse task resource usage header (binary and JSON), skipping: ", jsonFailure);
-            return null;
-        }
+    } catch (Exception e) {
+        logger.debug("failed to parse task resource usage header, skipping: ", e);
+        return null;
     }
 }
Suggestion importance[1-10]: 6

__

Why: Valid point: dispatching on the first character ({ for JSON vs. Base64) avoids throw/catch on the hot path during rolling upgrades, which is a reasonable performance and clarity improvement.

Low
Serialize JSON explicitly, not via toString

taskResourceInfo.toString() relies on the object's toString() producing valid JSON,
which is fragile and easy to break with a future refactor. Explicitly serialize via
Strings.toString(XContentType.JSON, taskResourceInfo) (or equivalent) to make the
wire format contract explicit and stable.

server/src/main/java/org/opensearch/tasks/TaskResourceTrackingService.java [364-366]

 String headerValue = canCoordinatorReadBinaryHeader(task)
     ? WriteableBase64.encode(taskResourceInfo)
-    : taskResourceInfo.toString();
+    : Strings.toString(MediaTypeRegistry.JSON, taskResourceInfo);
Suggestion importance[1-10]: 5

__

Why: Reasonable maintainability improvement — relying on toString() for wire format is fragile. However, the existing code preserves prior behavior so the change is not critical.

Low
Suggestions up to commit 337f418
CategorySuggestion                                                                                                                                    Impact
General
Add fallback for serialization failures

The serializeToBase64 method can throw IOException, but it's caught by the generic
Exception handler below. If serialization fails, the header won't be set,
potentially causing silent data loss. Consider adding explicit error handling or
falling back to JSON serialization when binary serialization fails to ensure the
header is always populated.

server/src/main/java/org/opensearch/tasks/TaskResourceTrackingService.java [367-369]

-String headerValue = canCoordinatorReadBinaryHeader(task) ? serializeToBase64(taskResourceInfo) : taskResourceInfo.toString();
+String headerValue;
+try {
+    headerValue = canCoordinatorReadBinaryHeader(task) ? serializeToBase64(taskResourceInfo) : taskResourceInfo.toString();
+} catch (IOException e) {
+    logger.debug("Binary serialization failed, falling back to JSON: ", e);
+    headerValue = taskResourceInfo.toString();
+}
 // Remove the existing TASK_RESOURCE_USAGE header since it would have come from an earlier phase in the same request.
 threadPool.getThreadContext().updateResponseHeader(TASK_RESOURCE_USAGE, headerValue);
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that serializeToBase64 can throw IOException, which could result in the header not being set. Adding explicit fallback to JSON serialization ensures the header is always populated, preventing potential data loss during task resource tracking.

Medium
Log binary deserialization failures

The binary deserialization failure is silently swallowed before attempting JSON
fallback. This makes debugging difficult when binary format issues occur. Log the
binary failure at debug level before falling back to JSON to aid troubleshooting
during rolling upgrades or format mismatches.

server/src/main/java/org/opensearch/tasks/TaskResourceTrackingService.java [421-430]

 try {
     return deserializeFromBase64(usage);
 } catch (Exception binaryFailure) {
+    logger.debug("Binary deserialization failed, attempting JSON fallback: ", binaryFailure);
     try {
         return deserializeFromJson(usage);
     } catch (Exception jsonFailure) {
         logger.debug("failed to parse task resource usage header (binary and JSON), skipping: ", jsonFailure);
         return null;
     }
 }
Suggestion importance[1-10]: 6

__

Why: Adding debug logging for binary deserialization failures before falling back to JSON would aid troubleshooting during rolling upgrades. However, this is a minor improvement since the final catch block already logs when both formats fail, and the fallback mechanism works correctly without this logging.

Low
Suggestions up to commit 1ed369e
CategorySuggestion                                                                                                                                    Impact
Possible issue
Add null check for parent task

Add null-safety check for task.getParentTaskId() before calling getNodeId(). If the
parent task ID is null, a NullPointerException will be thrown. This could occur in
edge cases where the task hierarchy is incomplete or malformed.

server/src/main/java/org/opensearch/tasks/TaskResourceTrackingService.java [360-373]

 private boolean canCoordinatorReadBinaryHeader(SearchShardTask task) {
     if (clusterService == null) {
         return false;
     }
-    String coordinatorNodeId = task.getParentTaskId().getNodeId();
+    TaskId parentTaskId = task.getParentTaskId();
+    if (parentTaskId == null) {
+        return false;
+    }
+    String coordinatorNodeId = parentTaskId.getNodeId();
     if (coordinatorNodeId.isEmpty()) {
         return false;
     }
     DiscoveryNode coordinator = clusterService.state().nodes().get(coordinatorNodeId);
     if (coordinator == null) {
         return false;
     }
     return coordinator.getVersion().onOrAfter(BINARY_RESOURCE_USAGE_HEADER_VERSION);
 }
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential NullPointerException if task.getParentTaskId() returns null. While the method already handles empty node IDs, it doesn't check for null TaskId. This is a valid defensive programming improvement that prevents runtime exceptions in edge cases.

Medium
General
Validate input parameter is not null

Add a null check for taskResourceInfo parameter to prevent NullPointerException
during serialization. If null is passed, the method will fail when calling
writeTo(), potentially causing unexpected failures in resource tracking.

server/src/main/java/org/opensearch/tasks/TaskResourceTrackingService.java [419-425]

 static String serializeToBase64(TaskResourceInfo taskResourceInfo) throws IOException {
+    if (taskResourceInfo == null) {
+        throw new IllegalArgumentException("taskResourceInfo cannot be null");
+    }
     try (BytesStreamOutput out = new BytesStreamOutput()) {
         taskResourceInfo.writeTo(out);
         byte[] bytes = BytesReference.toBytes(out.bytes());
         return Base64.getEncoder().encodeToString(bytes);
     }
 }
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that serializeToBase64 should validate its input parameter. Adding a null check with an IllegalArgumentException is a reasonable defensive practice that provides clearer error messages and prevents NullPointerException during serialization. However, this is a minor improvement since the method is package-private and likely called with valid inputs.

Medium
Suggestions up to commit b629cff
CategorySuggestion                                                                                                                                    Impact
Possible issue
Ensure most recent header value is read

The method retrieves only the first element (get(0)) from the list of resource usage
headers, but writeTaskResourceUsage uses updateResponseHeader which may accumulate
multiple values across phases. If there are multiple headers, only the first is
read, which may not be the most recent one. Verify whether the last element should
be used instead, or confirm that updateResponseHeader always replaces the previous
value so only one entry exists.

server/src/main/java/org/opensearch/tasks/TaskResourceTrackingService.java [345-358]

 public TaskResourceInfo getTaskResourceUsageFromThreadContext() {
     List<String> taskResourceUsages = threadPool.getThreadContext().getResponseHeaders().get(TASK_RESOURCE_USAGE);
     if (taskResourceUsages != null && taskResourceUsages.size() > 0) {
-        String usage = taskResourceUsages.get(0);
+        // Use the last entry as it represents the most recent phase's resource usage
+        String usage = taskResourceUsages.get(taskResourceUsages.size() - 1);
         try {
             if (usage != null && usage.isEmpty() == false) {
                 return deserializeFromBase64(usage);
             }
         } catch (Exception e) {
             logger.debug("failed to parse task resource usage header, skipping: ", e);
         }
     }
     return null;
 }
Suggestion importance[1-10]: 5

__

Why: The concern about get(0) vs get(size-1) is valid — if updateResponseHeader accumulates multiple values across phases, the first entry may be stale. However, the comment in the code says "Remove the existing TASK_RESOURCE_USAGE header since it would have come from an earlier phase," suggesting updateResponseHeader is intended to replace, not append. This warrants verification but is a legitimate concern.

Low
General
Avoid unnecessary byte array copy during serialization

The BytesReference.toBytes() call creates an extra byte array copy from the
BytesStreamOutput. You can avoid this unnecessary allocation by calling
out.bytes().toBytesRef().bytes or using out.bytes().streamInput() directly as the
source for the StreamInput in the deserializer, but more simply, use
out.bytes().toBytesRef() to get the backing array. However, the cleanest fix is to
use Base64.getEncoder().encode(BytesReference.toBytes(out.bytes())) — which is
what's already done — but note that BytesReference.toBytes() may copy. Consider
using out.bytes().streamInput() directly in a paired approach, or at minimum
document that this copy is intentional.

server/src/main/java/org/opensearch/tasks/TaskResourceTrackingService.java [368-374]

 static String serializeToBase64(TaskResourceInfo taskResourceInfo) throws IOException {
     try (BytesStreamOutput out = new BytesStreamOutput()) {
         taskResourceInfo.writeTo(out);
-        byte[] bytes = BytesReference.toBytes(out.bytes());
-        return Base64.getEncoder().encodeToString(bytes);
+        return Base64.getEncoder().encodeToString(out.bytes().toBytesRef().bytes);
     }
 }
Suggestion importance[1-10]: 2

__

Why: The suggestion proposes using out.bytes().toBytesRef().bytes instead of BytesReference.toBytes(out.bytes()), but toBytesRef().bytes may include extra capacity bytes beyond the actual content length, potentially encoding garbage data. The current approach with BytesReference.toBytes() is safer and the performance difference is negligible. The suggestion's "improved_code" could introduce a correctness bug.

Low
Suggestions up to commit 73ab078
CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix corrupted header test to match actual encoding format

The test uses a "bin:" prefix in the corrupted header value, but the production code
in serializeToBase64 does not add any such prefix — it encodes raw bytes directly.
This means the test is not accurately simulating a corrupted Base64 header; instead,
it should use a string that is invalid Base64 without any prefix (e.g.,
"not-valid-base64!!!").

server/src/test/java/org/opensearch/tasks/TaskResourceTrackingServiceTests.java [224-227]

-threadPool.getThreadContext().addResponseHeader(TASK_RESOURCE_USAGE, "bin:not-valid-base64!!!");
+threadPool.getThreadContext().addResponseHeader(TASK_RESOURCE_USAGE, "not-valid-base64!!!");
 
 TaskResourceInfo result = taskResourceTrackingService.getTaskResourceUsageFromThreadContext();
 assertNull("Corrupted binary header should return null gracefully", result);
Suggestion importance[1-10]: 6

__

Why: The test uses "bin:not-valid-base64!!!" as a corrupted header, but the production code doesn't add any "bin:" prefix. While the test still passes (both are invalid Base64), it's misleading and doesn't accurately simulate a corrupted Base64 header. The fix improves test accuracy.

Low
General
Handle Base64 decoding exceptions within declared throws

Base64.getDecoder() throws an IllegalArgumentException (not IOException) if the
input is not valid Base64. Since deserializeFromBase64 only declares throws
IOException, callers catching IOException will miss this runtime exception. The
calling code in getTaskResourceUsageFromThreadContext already catches Exception
broadly, but the method signature is misleading. Either catch and wrap
IllegalArgumentException inside the method, or document this behavior clearly.

server/src/main/java/org/opensearch/tasks/TaskResourceTrackingService.java [383-388]

 static TaskResourceInfo deserializeFromBase64(String headerValue) throws IOException {
-    byte[] bytes = Base64.getDecoder().decode(headerValue);
+    byte[] bytes;
+    try {
+        bytes = Base64.getDecoder().decode(headerValue);
+    } catch (IllegalArgumentException e) {
+        throw new IOException("Invalid Base64 header value", e);
+    }
     try (StreamInput in = StreamInput.wrap(bytes)) {
         return TaskResourceInfo.readFromStream(in);
     }
 }
Suggestion importance[1-10]: 4

__

Why: The observation is technically correct — Base64.getDecoder().decode() throws IllegalArgumentException which is not an IOException. However, the caller getTaskResourceUsageFromThreadContext already catches Exception broadly, so there's no functional bug. The fix improves method signature accuracy and makes the code more robust.

Low
Use padding-free Base64 encoding for HTTP headers

Using Base64.getEncoder() produces standard Base64 with padding characters (=),
which can be problematic in HTTP headers. Consider using Base64.getUrlEncoder() or
Base64.getEncoder().withoutPadding() to avoid any potential header parsing issues
with = characters.

server/src/main/java/org/opensearch/tasks/TaskResourceTrackingService.java [368-374]

 static String serializeToBase64(TaskResourceInfo taskResourceInfo) throws IOException {
     try (BytesStreamOutput out = new BytesStreamOutput()) {
         taskResourceInfo.writeTo(out);
         byte[] bytes = BytesReference.toBytes(out.bytes());
-        return Base64.getEncoder().encodeToString(bytes);
+        return Base64.getEncoder().withoutPadding().encodeToString(bytes);
     }
 }
Suggestion importance[1-10]: 3

__

Why: While = padding in Base64 can sometimes cause issues in HTTP headers, the updateResponseHeader method in OpenSearch's ThreadContext handles string values internally, making this a minor concern. The suggestion is valid but has low practical impact in this context.

Low

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 73ab078: TIMEOUT

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@dzane17
dzane17 force-pushed the binary-resource-usage-header branch from 73ab078 to b629cff Compare April 15, 2026 03:00
@github-actions

github-actions Bot commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 7d57080.

PathLineSeverityDescription
server/src/main/java/org/opensearch/tasks/TaskResourceTrackingService.java385mediumdeserializeFromBase64 performs binary deserialization of data sourced from response headers (ThreadContext) without explicit length/bounds validation before passing to StreamInput.wrap(). While headers originate from trusted cluster nodes, if a malicious or compromised node injects a crafted binary payload, it could trigger unexpected behavior in TaskResourceInfo.readFromStream depending on how that method handles malformed input. The risk is bounded by cluster trust, but worth auditing the readFromStream implementation for length checks.
server/src/main/java/org/opensearch/tasks/TaskResourceTrackingService.java344lowThe migration from JSON to binary format silently drops resource usage data (returns null) when a legacy JSON header is encountered, with no warning-level log or metric emitted. During rolling upgrades, nodes running old code send JSON headers that new nodes silently discard, making resource tracking gaps invisible to operators. While this is acknowledged in the Javadoc, silent data loss without observability signals could mask performance regressions or masquerade as a monitoring blind spot.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 1 | Low: 1


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit b629cff

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for b629cff: TIMEOUT

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@dzane17
dzane17 force-pushed the binary-resource-usage-header branch from b629cff to 7d57080 Compare May 7, 2026 17:58
@dzane17
dzane17 marked this pull request as ready for review May 7, 2026 17:58
@dzane17
dzane17 requested a review from a team as a code owner May 7, 2026 17:58

@ansjcy ansjcy 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.

LGTM, the numbers are pretty impressive. 2 caveats:

  • we need to accept the resource usgae data loss during rolling upgrade
  • Please add change log entry to reflect this.

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 15a1691

@jainankitk jainankitk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@kaushalmahi12 - What do you think about this change? Mostly looks good to me.

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 15a1691: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@dzane17
dzane17 requested a review from jainankitk May 18, 2026 22:09
@kkhatua

kkhatua commented May 19, 2026

Copy link
Copy Markdown
Member

ansjcy left a comment
LGTM, the numbers are pretty impressive. 2 caveats:
we need to accept the resource usgae data loss during rolling upgrade
Please add change log entry to reflect this.

@dzane17
Given this is a breaking change and you have UTs to cover the data loss during a rolling upgrade, were you able to verify this with an actual upgrade ?

I'm of the opinion that we should have a flag just incase something breaks and allow a rollback to the old format of the usage headers.

@dzane17
dzane17 force-pushed the binary-resource-usage-header branch from 15a1691 to 1ed369e Compare May 20, 2026 21:16
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 1ed369e

@github-actions github-actions Bot added enhancement Enhancement or improvement to existing feature or request Search:Query Insights labels May 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 337f418

@dzane17

dzane17 commented May 20, 2026

Copy link
Copy Markdown
Member Author

@kkhatua @ansjcy I decided to add bwc functionality so we will no longer will lose resource tracking info during upgrade and tested a local rolling upgrade. Details are in the description.

Also added a dynamic cluster setting to revert to JSON serialization in an emergency.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 337f418: SUCCESS

@codecov

codecov Bot commented May 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.18182% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.53%. Comparing base (a6b5e43) to head (337f418).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
.../opensearch/tasks/TaskResourceTrackingService.java 93.18% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #21230      +/-   ##
============================================
+ Coverage     73.43%   73.53%   +0.09%     
- Complexity    75103    75144      +41     
============================================
  Files          6016     6016              
  Lines        341072   341105      +33     
  Branches      49091    49095       +4     
============================================
+ Hits         250469   250834     +365     
+ Misses        70682    70281     -401     
- Partials      19921    19990      +69     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kkhatua

kkhatua commented Jun 5, 2026

Copy link
Copy Markdown
Member

Thanks, @dzane17

@kaushalmahi12 could you take a look at the PR?

Comment thread server/src/main/java/org/opensearch/tasks/TaskResourceTrackingService.java Outdated
dzane17 added 4 commits July 7, 2026 12:40
Signed-off-by: David Zane <davizane@amazon.com>
Signed-off-by: David Zane <davizane@amazon.com>
Signed-off-by: David Zane <davizane@amazon.com>
Signed-off-by: David Zane <davizane@amazon.com>
@dzane17
dzane17 force-pushed the binary-resource-usage-header branch from 337f418 to f416371 Compare July 7, 2026 20:28
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit f416371

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for f416371: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for f416371: TIMEOUT

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: David Zane <davizane@amazon.com>
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 2e2537a

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 2e2537a: SUCCESS

@cwperks
cwperks merged commit fe5d619 into opensearch-project:main Jul 8, 2026
13 checks passed
@dzane17
dzane17 deleted the binary-resource-usage-header branch July 8, 2026 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Enhancement or improvement to existing feature or request Search:Query Insights

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request] Optimizing Resource Usage Header Performance for Search Requests

5 participants