Remove PROTO-based custom cluster state components #22336
Conversation
There was a problem hiding this comment.
Is there an incomplete NamedDiff?
There was a problem hiding this comment.
No, but there could be partial NamedDiff. CompleteNameDiff is a NamedWriteable equivalent of CompleteDiff which sends a complete copy every time anything changes, hence the name.
There was a problem hiding this comment.
Comments and class name are now out of date.
There was a problem hiding this comment.
Good catch. I am going to rename it to DiffableValueReader
There was a problem hiding this comment.
Can you use the constructor instead?
There was a problem hiding this comment.
I see the symmetry of having these two methods. I still wonder if it'd be as readable if you removed them and used the ctors directly.
There was a problem hiding this comment.
I didn't move readFrom to constructor to reduce the number of changes. I will do that. I wouldn't want to move code in readDiffFrom away from the class itself, because it will expose too much implementation details in the registration code.
There was a problem hiding this comment.
Do we need this method any more?
There was a problem hiding this comment.
I think instead we can use the one it delegates to, right?
There was a problem hiding this comment.
Oops. These are leftovers from when I was trying to convert custom index metadata to the new mode. I will restore this back.
There was a problem hiding this comment.
Can it have a reading ctor? Those tend to do nice stuff like let you declare more things final.
There was a problem hiding this comment.
do we still want to warn here? Supposedly this only occurs when you uninstall a plugin that uses customer state, right? I think it'd be cool to log a warning.
There was a problem hiding this comment.
I'd add something like Prefer {@link StreamInput#readNamedWriteable(Class)} and {@link StreamOutput#writeNamedWriteable(NamedWriteable)} unless you have a compelling reason to use this method instead..
There was a problem hiding this comment.
Maybe use NamedXContentRegistry.FromXContent instead. Maybe extract it into a top level class? Remember that ParseFieldMatcher is being removed. If you want to keep the signatures the same you can use the static ParseFieldMatcher.EMPTY or remove all usages.
|
@nik9000 I pushed changes that you have requested. Could you take another look when you have a chance? |
martijnvg
left a comment
martijnvg
left a comment
There was a problem hiding this comment.
LGTM
Maybe we should test backwards compatibility in rolling upgrade qa test, just to be sure?
Something like I did in #21243? First add some custom cluster state components, then verify that the custom cluster state parts exist in both mixed and upgraded cluster. We do then need to [preserve the cluster state] (https://github.com/elastic/elasticsearch/pull/21243/files#diff-701d16f5dd09c2af203aed8b5838c22cR40) between tests.
Switches custom cluster state components from PROTO-based de-serialization to named objects based de-serialization
imotov
force-pushed
the
issue-21868-kill-proto-rebased
branch
from
f584231 to
ca90d9e
Compare
clintongormley
added
:Distributed Indexing/Distributed
4 participants
Switches custom cluster state components from PROTO-based de-serialization to named objects based de-serialization
Closes #21868
This PR removes PROTO-based serialization from all custom objects except custom IndexMetaData. Custom IndexMetaData is not currently used in any know code and will be removed in v7.0. I will open a separate PR deprecating it and disabling creation of Custom IndexMetaData for new indices in v6.0.