Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce chunked version of ToXContentObject #92549

Conversation

original-brownbear
Copy link
Member

@original-brownbear original-brownbear commented Dec 23, 2022

Aligning how chunked and normal ToXContent work here and simplifying spots where it matters whether or not something is a fragment or not.
This fixes broken toString for all kinds of classes and adds a test that makes sure the implementation at least produces a result and doesn't throw.

Aligning how chunked and normal `ToXContent` work here and simplifying
spots where it matters whether or not something is a fragment or not.
@original-brownbear original-brownbear added WIP :Core/Infra/Core Core issues without another label v8.7.0 labels Dec 23, 2022
@@ -162,4 +162,8 @@ public int hashCode() {
return Objects.hash(super.hashCode(), reduceScript, aggregations);
}

@Override
public String toString() {
return "InternalScriptedMetric{" + reduceScript + "}";
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to do something here since the parent class's toString would call back to toXContent here which would throw if the instance hasn't yet been reduced.

@@ -44,12 +44,13 @@ private static Map<String, DiskUsage> randomDiskUsage() {
Map<String, DiskUsage> builder = new HashMap<>(numEntries);
for (int i = 0; i < numEntries; i++) {
String key = randomAlphaOfLength(32);
final int totalBytes = randomIntBetween(0, Integer.MAX_VALUE);
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to fix this to use plausible values, otherwise toString would run into negative byte values which would throw illegal argument ex.

@@ -120,10 +118,4 @@ public void testToXContent() throws IOException {
}"""), xContent.utf8ToString());
}

public void testToString() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to the parent now to deal with this for all the things we serialize

@@ -241,6 +241,9 @@ public int hashCode() {

@Override
public String toString() {
if (indexResponses.size() > 0) {
return "FieldCapabilitiesResponse{unmerged}";
Copy link
Member Author

Choose a reason for hiding this comment

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

Not great but we shouldn't throw on toString ever so we have to have some breakout here for the un-merged case that doesn't allow toXContent.

@original-brownbear
Copy link
Member Author

Jenkins run elasticsearch-ci/part-3

@original-brownbear original-brownbear marked this pull request as ready for review December 27, 2022 14:54
@original-brownbear
Copy link
Member Author

@DaveCTurner I think this is the direction I'd like to go before we add more stuff around fragments. Seems easiest to just align with ToXContent for now ...

@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Dec 27, 2022
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Makes sense, yes.

Why not replace org.elasticsearch.common.xcontent.ChunkedToXContent#wrapAsXContentObject with something that returns a ToXContent with the appropriate isFragment value?

@original-brownbear
Copy link
Member Author

@DaveCTurner ++ good point, that alley for even more simplifications. See f64ad95 :)

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM (one naming nit)

@original-brownbear
Copy link
Member Author

Thanks David!

@original-brownbear original-brownbear merged commit cc5ef3b into elastic:main Dec 28, 2022
@original-brownbear original-brownbear deleted the fragments-for-chunked-to-xcontent branch December 28, 2022 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants