Make feature usage version aware #55246
Conversation
Today we indiscriminately serialize these independent of the version on the stream, even though the other side might not understand a new feature set usage that we have added. For example, if we add feature set usage in 7.7 for EQL, in a mixed cluster context if a request is sent to an old coordinating node, but the master is a new version, then it would attempt to serialize the usage information for the new feature back to the old coordinating node, who will blow up on the unrecognized named writeable. This commit addresses this by making feature usage version aware, and only serializing those that the other side would understand.
jasontedor
added
>bug
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (:Core/Infra/Core) |
There was a problem hiding this comment.
It looks good, but I wonder if Usage should implement VersionedNamedWriteable and if it makes sense to add readVersionedNamedWriteableList and writeVersionedNamedWriteableList to StreamInput/Output. What do you think?
|
Thanks for the great review @imotov! |
There was a problem hiding this comment.
Overall this seems fine, but I'm concerned it will add yet another thing that must be updated when we branch a major version, since these then all need to be updated to the new minimum version constant. Instead of Version.V_7_0_0, can we use Version.CURRENT.minimumCompatibilityVersion() as the default in the base class, so updating is not necessary except to remove overrides for something that was previously added in a minor version?
|
I was going to make a very similar comment to @rjernst but I was concerned that that result of this will be us forgetting to implement this method on a newly added classes. So I actually decided that having this method unimplemented is a feature (On the other side it should be caught in the test, so I don't have strong feelings about it) |
|
I'm not concerned about missing it, since as you point out, mixed cluster tests should catch the inconsistency. |
|
I do not think we should do this, as testing only captures this failure scenario randomly; the failure scenario for the one that led to this (EQL) has been in place for nearly two months but we didn't have any tests triggering the scenario. It feels like a bad tradeoff to me to use minimum compatibility version just for development convenience to not have to update a few version constants the one time every "many" months that we bump the major version versus forcing developers to implement this method with the right version constant when we add a new feature. Maybe if we had better testing here I would feel better, but we don't, we're lucky we caught this at all via some other tests that were added yesterday. |
|
@elasticmachine update branch |
Today we indiscriminately serialize these independent of the version on the stream, even though the other side might not understand a new feature set usage that we have added. For example, if we add feature set usage in 7.7 for EQL, in a mixed cluster context if a request is sent to an old coordinating node, but the master is a new version, then it would attempt to serialize the usage information for the new feature back to the old coordinating node, who will blow up on the unrecognized named writeable. This commit addresses this by making feature usage version aware, and only serializing those that the other side would understand.
Today we indiscriminately serialize these independent of the version on the stream, even though the other side might not understand a new feature set usage that we have added. For example, if we add feature set usage in 7.7 for EQL, in a mixed cluster context if a request is sent to an old coordinating node, but the master is a new version, then it would attempt to serialize the usage information for the new feature back to the old coordinating node, who will blow up on the unrecognized named writeable. This commit addresses this by making feature usage version aware, and only serializing those that the other side would understand.
Today we indiscriminately serialize these independent of the version on the stream, even though the other side might not understand a new feature set usage that we have added. For example, if we add feature set usage in 7.7 for EQL, in a mixed cluster context if a request is sent to an old coordinating node, but the master is a new version, then it would attempt to serialize the usage information for the new feature back to the old coordinating node, who will blow up on the unrecognized named writeable. This commit addresses this by making feature usage version aware, and only serializing those that the other side would understand.
Closes #44589
Closes #55248