-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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.
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
[refactor] Migrate Usages of Streamable to Writeable #34389
Comments
Pinging @elastic/es-core-infra |
I think we need a migration plan for Response objects. As far as I can tell we are missing the framework for an
I do wonder if this is the right time to make these objects immutable; if there is a builder we can add mutable fields in the builder and build the immutable request object upon calling |
Are you saying you'd like to make these objects immutable as we go? I'm on board with that, though superclasses make that hard. But it is still a nice goal. Maybe not for (some) requests though, because we still use them in the high level rest client and transport client. Once we've stopped using them in the high level rest client I think we should make them immutable. |
I think we need a refactoring meta-issue for this effort too. |
Yes I would like to do it as much as possible and understand that in some places it might not be worth trying to do as we go like if there are superclasses that conflict with making things immutable. It has always bothered me that request/responses could be mutable. |
This commit replaces usage of Streamable with Writeable within the RoleDescriptor class (and inner classes). Relates: elastic#34389
This commit replaces usage of Streamable with Writeable within the RoleDescriptor class (and inner classes). Relates: #34389
In order to remove Streamable from the codebase, Response objects need to be read using the Writeable.Reader interface which this change enables. This change enables the use of Writeable.Reader by adding the `Action#getResponseReader` method. The default implementation simply uses the existing `newResponse` method and the readFrom method. As responses are migrated to the Writeable.Reader interface, Action classes can be updated to throw an UnsupportedOperationException when `newResponse` is called and override the `getResponseReader` method. Relates elastic#34389
This commit replaces usage of Streamable with Writeable within the RoleDescriptor class (and inner classes). Relates: #34389
In order to remove Streamable from the codebase, Response objects need to be read using the Writeable.Reader interface which this change enables. This change enables the use of Writeable.Reader by adding the `Action#getResponseReader` method. The default implementation simply uses the existing `newResponse` method and the readFrom method. As responses are migrated to the Writeable.Reader interface, Action classes can be updated to throw an UnsupportedOperationException when `newResponse` is called and override the `getResponseReader` method. Relates #34389
In order to remove Streamable from the codebase, Response objects need to be read using the Writeable.Reader interface which this change enables. This change enables the use of Writeable.Reader by adding the `Action#getResponseReader` method. The default implementation simply uses the existing `newResponse` method and the readFrom method. As responses are migrated to the Writeable.Reader interface, Action classes can be updated to throw an UnsupportedOperationException when `newResponse` is called and override the `getResponseReader` method. Relates #34389
With the incorporation of #34655, Response objects can now use the Lines 45 to 62 in 052dfa5
The Lines 64 to 67 in 052dfa5
The action class, Lines 39 to 42 in 052dfa5
Finally, the Lines 34 to 37 in 052dfa5
|
This commit replaces usage of Streamable with Writeable within the RoleDescriptor class (and inner classes). Relates: #34389
In order to remove Streamable from the codebase, Response objects need to be read using the Writeable.Reader interface which this change enables. This change enables the use of Writeable.Reader by adding the `Action#getResponseReader` method. The default implementation simply uses the existing `newResponse` method and the readFrom method. As responses are migrated to the Writeable.Reader interface, Action classes can be updated to throw an UnsupportedOperationException when `newResponse` is called and override the `getResponseReader` method. Relates #34389
This commit replaces usages of Streamable with Writeable for the BaseTasksRequest / TransportTasksAction classes and subclasses of these classes. Relates to elastic#34389
This commit replaces usages of Streamable with Writeable for the BaseTasksResponse / TransportTasksAction classes and subclasses of these classes. Note that where possible response fields were made final. Relates to elastic#34389
This commit moves the Supplier variant of HandledTransportAction to have a different ordering than the Writeable.Reader variant. The Supplier version is used for the legacy Streamable, and currently having the location of the Writeable.Reader vs Supplier in the same place forces using casts of Writeable.Reader to select the correct super constructor. This change in ordering allows easier migration to Writeable.Reader. relates elastic#34389
This commit creates new base classes for master node actions whose response types still implement Streamable. This simplifies both finding remaining classes to convert, as well as creating new master node actions that use Writeable for their responses. relates elastic#34389
) This commit converts all BaseNodeResponse and BaseNodesResponse subclasses to implement Writeable.Reader instead of Streamable. relates elastic#34389
) This commit converts all the StreamableResponseActionType security classes in xpack core to ActionType, implementing Writeable for their response classes. relates elastic#34389
This commit cleans up replication response and request so that the base class does not allow subclasses to implement Streamable. relates elastic#34389
…ic#44386) This commit converts the request and response classes for broadcast actions to implement ctors for Writeable.Reader and forces all future implementations to implement the same. relates elastic#34389
This commit adds constructors to AcknolwedgedRequest subclasses to implement Writeable.Reader, and ensures all future subclasses implement the same. relates elastic#34389
This PR converts `ClearScrollRequest` and `ClearScrollResponse` to `Writeable`. Relates to elastic#34389.
This commit converts all MasterNodeRequest subclasses to fullfill Writeable.Reader constructors. relates elastic#34389
This commit converts all remaining ActionType response classes to writeable in xpack core. It also converts a few from server which were used by xpack core. relates elastic#34389
…#44464) this commit migrates leftover actions from a few x-pack plugins to the new Writeable.Reader infrastructure. relates elastic#34389.
this commit removes usage of the deprecated constructor with a single argument and no Writeable.Reader. The purpose of this is to reduce the boilerplate necessary for properly implementing a new action, as well as reducing the chances of using the incorrect super constructor while classes are being migrated to Writeable relates elastic#34389.
* Convert FieldCapabilities*. * Convert MultiTermVectors*. * Convert SyncedFlush*. * Convert SearchTemplateRequest. * Convert MultiSearchTemplateRequest. * Convert GrokProcessorGet*. Relates to elastic#34389.
many classes still use the Streamable constructors of HandledTransportAction, this commit moves more of those classes to the new Writeable constructors. relates elastic#34389.
…lastic#44456) This commit deprecates all constructors of HandledTransportAction that take in a Supplier instead of a Writeable.Reader for response objects. in addition to the deprecation, the following modules were updated to leverage Writeable - modules:ingest-common - modules:lang-mustache relates elastic#34389.
…lastic#44524) This commit converts all remaining classes extending ActionRequest in xpack core to have a StreamInput constructor. relates elastic#34389
This commit converts subclasses of ShardOperationFailedException to implement ctors with StreamInput instead of readFrom. It also simplifies IndicesShardStoresResponse.Failure to serialize its shardId after the super data. relates elastic#34389
…ic#44578) This commit removes references to Streamable from StreamInput. This is all a part of the effort to remove Streamable usage. relates elastic#34389.
* Convert GetTask*. * Convert RemoteInfo*. * Convert GetFieldMappings*. * Convert ValidateQueryRequest*. * Convert MainResponse*. * Convert MultiGet*. * Convert Update*. Relates to elastic#34389.
…4582) This commit converts several more classes from streamable to writeable in server, mostly within the o.e.index and o.e.persistent packages. relates elastic#34389
This commit converts several utility classes that implement Streamable to have StreamInput constructors. It also adds a default version of readFrom to Streamable so that overriding to throw UOE is not necessary. relates elastic#34389
…c#44528) This commit converts readFrom to ctor with StreamInput on the remaining ActionResponse and ActionRequest classes. relates elastic#34389
…elastic#44602) remove usages of writeOptionalStreamable and writeStreambaleList relates elastic#34389.
) This commit converts Streamable to Writeable for direct implementations. relates elastic#34389
This commit converts all remaining TransportRequest and TransportResponse classes to implement Writeable, and disallows Streamable implementations. relates elastic#34389
This commit ends the grand adventure that was the refactoring effort to migrate all usages of Streamable to Writeable. Closes elastic#34389.
Reason for migration
Implementers of the Streamable interface almost always declare a no arg constructor that is exclusively used for creating "empty" objects on which you then call
#readFrom(StreamInput)
. Because#readFrom(StreamInput)
isn't part of the constructor the fields on implementers cannot be final. It is these reasons that this interface has fallen out of favor compared to WriteableMigration plan
Some steps have already begun:
Next Steps:
Example Migration
Let's assume there is a concrete class that does not have any constraints via inheritance, and can simply be migrated from Streamable to Writeable, it is best to do the following:
readFrom
method override with a method that throws an exception. This ensures that the Streamable interface for this class is never actually used.readFrom
implementation into a Writeable.Reader-compatible constructorfinal
Classes (757 as of opening this issue) that implement Streamble
Class Hierarchy
The text was updated successfully, but these errors were encountered: