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.

Sign up for GitHub

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

Jump to bottom

PutShutdownNodeAction.Request does not preserve parent task or timeouts on the wire #107857

Closed
DaveCTurner opened this issue Apr 24, 2024 · 2 comments · Fixed by #107862
Closed

PutShutdownNodeAction.Request does not preserve parent task or timeouts on the wire #107857

DaveCTurner opened this issue Apr 24, 2024 · 2 comments · Fixed by #107862
Labels
>bug :Core/Infra/Node Lifecycle Node startup, bootstrapping, and shutdown Team:Core/Infra Meta label for core/infra team

Comments

@DaveCTurner
Copy link
Contributor

PutShutdownNodeAction.Request is an AcknowledgedRequest whose transport-protocol representation therefore should start with the parent task ID followed by the ?masterNodeTimeout and ?timeout parameters. However we do not call the superclass writeTo() method when serializing it so these fields are omitted, and therefore we end up with the default 30s timeouts (and no parent task ID) on a remote master node.
@DaveCTurner DaveCTurner added >bug :Core/Infra/Node Lifecycle Node startup, bootstrapping, and shutdown labels Apr 24, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Apr 24, 2024
@elasticsearchmachine
Copy link
Collaborator

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

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this issue Apr 24, 2024
@DaveCTurner
Fix serialization of put-shutdown request
5270a2b 
Closes elastic#107857
@DaveCTurner DaveCTurner mentioned this issue Apr 24, 2024
Merged
@DaveCTurner
Copy link
Contributor Author

the same bug applies to DeleteShutdownNodeAction.Request.

@DaveCTurner DaveCTurner mentioned this issue Apr 28, 2024
Open
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this issue May 8, 2024
@DaveCTurner
Make master-node timeout less implicit
d2c5a4a 
Removes the default constructors for `MasterRequest`,
`MasterReadRequest` and `AcknowledgedRequest` in favour of constructors
which require subclasses to specify the relevant timeouts. This will
avoid bugs like elastic#107857 which are caused by a missing `super()` call.

Also deprecates and renames the default to make it clear it should not
be used in new code.

Relates elastic#107984
@DaveCTurner DaveCTurner mentioned this issue May 8, 2024
Merged
elasticsearchmachine pushed a commit that referenced this issue May 13, 2024
@DaveCTurner
Make master-node timeout less implicit (#108414)
0b2e558 
Removes the default constructors for `MasterRequest`,
`MasterReadRequest` and `AcknowledgedRequest` in favour of constructors
which require subclasses to specify the relevant timeouts. This will
avoid bugs like #107857 which are caused by a missing `super()` call.

Also deprecates and renames the default to make it clear it should not
be used in new code.

Relates #107984
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
>bug :Core/Infra/Node Lifecycle Node startup, bootstrapping, and shutdown Team:Core/Infra Meta label for core/infra team
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix serialization of put/delete shutdown requests DaveCTurner/elasticsearch
2 participants
@DaveCTurner @elasticsearchmachine