Conversation
Fixes trappy timeouts related to elastic#107984 and also cleans up all the indirection via unnecessary request builders.
|
Pinging @elastic/es-security (Team:Security) |
jakelandis
left a comment
There was a problem hiding this comment.
Thanks for addressing this. LGTM, but a couple follow up questions (I can follow up).
Not sure I understand the difference between the timeout applied on the MasterNodeRequest vs. the submitTask. Should we be passing in request.masterNodeTimeout() to the MasterServiceTaskQueue#submitTask in the following:
https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/core/src/main/java/org/elasticsearch/license/ClusterStateLicenseService.java#L317
https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/core/src/main/java/org/elasticsearch/license/ClusterStateLicenseService.java#L348
https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/core/src/main/java/org/elasticsearch/license/ClusterStateLicenseService.java#L362
Also, I am correct to assume that the ackTimeout (w.r.t. cluster state updates) only applies for AckedClusterStateUpdateTask and AckedBatchedClusterStateUpdateTask tasks ? I think PostStartBasicRequest could be a MasterNodeRequest instead of a AcknowledgedRequest since it is the wrong kind of "acknowledged" since StartBasicClusterTask is not acked.
The timeout on the
Yes. Probably. The trouble is that today once these tasks make it to the elected master they will today sit in the queue for as long as it takes to execute them. That's likely an oversight, but it's one which dates back a very long way. Changing this to align with the common
Technically it applies to all tasks that implement
Possibly, but possibly also the tasks which adjust the license should be made into acked tasks? It'd make sense to me to have these tasks wait for the updated license to be applied on all nodes, rather than just on the master as happens today. |
Fixes trappy timeouts related to #107984 and also cleans up all the
indirection via unnecessary request builders.