Skip to content

Improve Netty4IncrementalRequestHandlingIT #127111

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

Conversation

DaveCTurner
Copy link
Contributor

  • Verifies that each call to Netty4HttpRequestBodyStream#next yields
    exactly one chunk (or the stream is closed) since the
    IncrementalBulkService relies on this property.

  • Replaces several busy-waits with ones that block on a future for
    faster test execution.

  • Replaces several hard-coded constants with randomized values to
    clarify that the precise value does not matter to the test.

  • Reduces the use of unnecessary abbreviations in names.

  • Reduce the use of global static state in favour of node-local
    components.

* Verifies that each call to `Netty4HttpRequestBodyStream#next` yields
  exactly one chunk (or the stream is closed) since the
  `IncrementalBulkService` relies on this property.

* Replaces several busy-waits with ones that block on a future for
  faster test execution.

* Replaces several hard-coded constants with randomized values to
  clarify that the precise value does not matter to the test.

* Reduces the use of unnecessary abbreviations in names.

* Reduce the use of global static state in favour of node-local
  components.
@DaveCTurner DaveCTurner added >test Issues or PRs that are addressing/adding tests :Distributed Coordination/Network Http and internode communication implementations v9.1.0 labels Apr 21, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Apr 21, 2025
@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Contributor

@mhl-b mhl-b left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@nicktindall nicktindall left a comment

Choose a reason for hiding this comment

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

LGTM

}
assertBusy(() -> assertEquals("should receive all server responses", totalRequests, ctx.clientRespQueue.size()));
}
}

// ensures content integrity, no loses and re-order
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit typo "losses"

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Apr 22, 2025
@elasticsearchmachine elasticsearchmachine merged commit f9d813a into elastic:main Apr 22, 2025
17 checks passed
@DaveCTurner DaveCTurner deleted the 2025/04/20/netty-incremental-request-processing-chunk-at-once branch April 22, 2025 11:37
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Apr 23, 2025
Re-applying elastic#126441 with the extra `FlowControlHandler` needed to ensure
one-chunk-per-read semantics - see elastic#127111 for related tests.
DaveCTurner added a commit that referenced this pull request Apr 25, 2025
Re-applying #126441 with the extra `FlowControlHandler` needed to ensure
one-chunk-per-read semantics - see #127111 for related tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed Coordination/Network Http and internode communication implementations Team:Distributed Coordination Meta label for Distributed Coordination team >test Issues or PRs that are addressing/adding tests v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants