Skip to content

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

}
}

// 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.
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request May 7, 2025
Re-applying elastic#126441 (cf. elastic#127259) with:

- the extra `FlowControlHandler` needed to ensure one-chunk-per-read
  semantics (also present in elastic#127259).

- no extra `read()` after exhausting a `Netty4HttpRequestBodyStream`
  (the bug behind elastic#127391 and elastic#127391).

See elastic#127111 for related tests.
elasticsearchmachine pushed a commit that referenced this pull request May 8, 2025
Re-applying #126441 (cf. #127259) with:

- the extra `FlowControlHandler` needed to ensure one-chunk-per-read
  semantics (also present in #127259).

- no extra `read()` after exhausting a `Netty4HttpRequestBodyStream`
  (the bug behind #127391 and #127391).

See #127111 for related tests.
ywangd pushed a commit to ywangd/elasticsearch that referenced this pull request May 9, 2025
…127817)

Re-applying elastic#126441 (cf. elastic#127259) with:

- the extra `FlowControlHandler` needed to ensure one-chunk-per-read
  semantics (also present in elastic#127259).

- no extra `read()` after exhausting a `Netty4HttpRequestBodyStream`
  (the bug behind elastic#127391 and elastic#127391).

See elastic#127111 for related tests.
jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request May 12, 2025
…127817)

Re-applying elastic#126441 (cf. elastic#127259) with:

- the extra `FlowControlHandler` needed to ensure one-chunk-per-read
  semantics (also present in elastic#127259).

- no extra `read()` after exhausting a `Netty4HttpRequestBodyStream`
  (the bug behind elastic#127391 and elastic#127391).

See elastic#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