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

Always flush response body in AbstractBlobContainerRetriesTestCase#sendIncompleteContent with JDK23 #115197

Conversation

pxsalehi
Copy link
Member

Resolves #115172

@pxsalehi pxsalehi added >test Issues or PRs that are addressing/adding tests :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Oct 21, 2024
@elasticsearchmachine elasticsearchmachine added Team:Distributed Meta label for distributed team (obsolete) v9.0.0 labels Oct 21, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@@ -418,7 +418,9 @@ protected int sendIncompleteContent(HttpExchange exchange, byte[] bytes) throws
if (bytesToSend > 0) {
exchange.getResponseBody().write(bytes, rangeStart, bytesToSend);
}
if (randomBoolean()) {
if (randomBoolean() || Runtime.version().feature() >= 23) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The "fix" should be only temporary and for a specific version, so I'd rather not always flush.

@pxsalehi pxsalehi requested a review from ywangd October 21, 2024 09:50
Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

LGTM

I think you'll want to backport this to 7.x?

Comment on lines 422 to 423
// For now in JDK23 we need to always flush. See https://bugs.openjdk.org/browse/JDK-8331847.
// TODO: remove the JDK version check once that bug is fixed
Copy link
Member

Choose a reason for hiding this comment

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

It's not a bug but rather intended behaviour as an optimization if I understand the jdk issue correctly.

@pxsalehi pxsalehi added auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) auto-backport Automatically create backport pull requests when merged labels Oct 21, 2024
@elasticsearchmachine elasticsearchmachine merged commit 671458a into elastic:main Oct 21, 2024
16 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

The backport operation could not be completed due to the following error:

An unexpected error occurred when attempting to backport this PR.

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 115197

@pxsalehi pxsalehi deleted the ps241021-fix-testReadBlobWithReadTimeouts branch October 21, 2024 11:02
pxsalehi added a commit to pxsalehi/elasticsearch that referenced this pull request Oct 21, 2024
pxsalehi added a commit to pxsalehi/elasticsearch that referenced this pull request Oct 21, 2024
This was referenced Oct 21, 2024
elasticsearchmachine pushed a commit that referenced this pull request Oct 21, 2024
pxsalehi added a commit that referenced this pull request Oct 22, 2024
mark-vieira pushed a commit to mark-vieira/elasticsearch that referenced this pull request Oct 23, 2024
elasticsearchmachine pushed a commit that referenced this pull request Oct 23, 2024
…ndIncompleteContent with JDK23 (#115197) (#115441)

Resolves #115172

Co-authored-by: Pooya Salehi <[email protected]>
georgewallace pushed a commit to georgewallace/elasticsearch that referenced this pull request Oct 25, 2024
mark-vieira pushed a commit to mark-vieira/elasticsearch that referenced this pull request Oct 25, 2024
elasticsearchmachine pushed a commit that referenced this pull request Oct 25, 2024
…ndIncompleteContent with JDK23 (#115197) (#115680)

Resolves #115172

Co-authored-by: Pooya Salehi <[email protected]>
jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) backport pending :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed Meta label for distributed team (obsolete) >test Issues or PRs that are addressing/adding tests v7.17.26 v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] GoogleCloudStorageBlobContainerRetriesTests testReadBlobWithReadTimeouts failing
3 participants