Skip to content

Revert Remove direct cloning of BytesTransportRequests#117200

Merged
Tim-Brooks merged 2 commits intoelastic:mainfrom
Tim-Brooks:do_not_assert_bytes_class
Nov 20, 2024
Merged

Revert Remove direct cloning of BytesTransportRequests#117200
Tim-Brooks merged 2 commits intoelastic:mainfrom
Tim-Brooks:do_not_assert_bytes_class

Conversation

@Tim-Brooks
Copy link
Copy Markdown
Contributor

@Tim-Brooks Tim-Brooks commented Nov 20, 2024

Reverts #114808 and unmutes
#117024 which was a related failure.

@Tim-Brooks Tim-Brooks added >test Issues or PRs that are addressing/adding tests :Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. v9.0.0 labels Nov 20, 2024
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Distributed Indexing (obsolete) Meta label for Distributed Indexing team. Obsolete. Please do not use. label Nov 20, 2024
@DaveCTurner
Copy link
Copy Markdown
Member

Ah, no, this is a bug introduced by #114808. Pretty surprising it's taken this long before anyone noticed. Could you revert #114808 to reinstate the @UpdateForV9 annotation to remind us to address this properly before we release v9? /cc @arteam

@Tim-Brooks Tim-Brooks force-pushed the do_not_assert_bytes_class branch from 5301bcf to 26588fd Compare November 20, 2024 21:28
@Tim-Brooks Tim-Brooks changed the title Do not assert class type when request is bytes Revert "Remove direct cloning of BytesTransportRequests Nov 20, 2024
@Tim-Brooks Tim-Brooks changed the title Revert "Remove direct cloning of BytesTransportRequests Revert Remove direct cloning of BytesTransportRequests Nov 20, 2024
@Tim-Brooks
Copy link
Copy Markdown
Contributor Author

Tim-Brooks commented Nov 20, 2024

Ah, no, this is a bug introduced by #114808. Pretty surprising it's taken this long before anyone noticed. Could you revert #114808 to reinstate the @UpdateForV9 annotation to remind us to address this properly before we release v9? /cc @arteam

Sure. Reverted and test unmuted.

Copy link
Copy Markdown
Member

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

Thanks Tim LGTM

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Nov 20, 2024
Since 8.3.0 (elastic#85380) we have sent join-validation requests as a
`BytesTransportRequest` to facilitate sharing these large messages (and
the work needed to create them) amongst all nodes that join the cluster
at around the same time. For BwC with versions earlier than 8.3.0 we use
a `ValidateJoinRequest` class to represent the received data, whichever
scheme it uses. We no longer need to maintain this compatibility, so we
can use a bare `BytesTransportRequest` on both sender and receiver, and
therefore drop the `ValidateJoinRequest` adapter and the special-cased
assertion in `MockTransportService`.

Relates elastic#114808 which was reverted in elastic#117200.
@Tim-Brooks Tim-Brooks merged commit 4cc9f5d into elastic:main Nov 20, 2024
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Nov 21, 2024
Since 8.3.0 (elastic#85380) we have sent join-validation requests as a
`BytesTransportRequest` to facilitate sharing these large messages (and
the work needed to create them) amongst all nodes that join the cluster
at around the same time. For BwC with versions earlier than 8.3.0 we use
a `ValidateJoinRequest` class to represent the received data, whichever
scheme it uses. We no longer need to maintain this compatibility, so we
can use a bare `BytesTransportRequest` on both sender and receiver, and
therefore drop the `ValidateJoinRequest` adapter and the special-cased
assertion in `MockTransportService`.

Relates elastic#114808 which was reverted in elastic#117200.
elasticsearchmachine pushed a commit that referenced this pull request Nov 21, 2024
Since 8.3.0 (#85380) we have sent join-validation requests as a
`BytesTransportRequest` to facilitate sharing these large messages (and
the work needed to create them) amongst all nodes that join the cluster
at around the same time. For BwC with versions earlier than 8.3.0 we use
a `ValidateJoinRequest` class to represent the received data, whichever
scheme it uses. We no longer need to maintain this compatibility, so we
can use a bare `BytesTransportRequest` on both sender and receiver, and
therefore drop the `ValidateJoinRequest` adapter and the special-cased
assertion in `MockTransportService`.

Relates #114808 which was reverted in #117200.
alexey-ivanov-es pushed a commit to alexey-ivanov-es/elasticsearch that referenced this pull request Nov 28, 2024
alexey-ivanov-es pushed a commit to alexey-ivanov-es/elasticsearch that referenced this pull request Nov 28, 2024
Since 8.3.0 (elastic#85380) we have sent join-validation requests as a
`BytesTransportRequest` to facilitate sharing these large messages (and
the work needed to create them) amongst all nodes that join the cluster
at around the same time. For BwC with versions earlier than 8.3.0 we use
a `ValidateJoinRequest` class to represent the received data, whichever
scheme it uses. We no longer need to maintain this compatibility, so we
can use a bare `BytesTransportRequest` on both sender and receiver, and
therefore drop the `ValidateJoinRequest` adapter and the special-cased
assertion in `MockTransportService`.

Relates elastic#114808 which was reverted in elastic#117200.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. Team:Distributed Indexing (obsolete) Meta label for Distributed Indexing team. Obsolete. Please do not use. >test Issues or PRs that are addressing/adding tests v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants