Remove redundant writeList from StreamOutput #98971
Conversation
Noticed this when benchmarking FieldCaps transport messages. The `writeList` alias just adds more lines to the code and makes profiling more annoying to read, lets remove it.
original-brownbear
added
>non-issue
:Core/Infra/Core
elasticsearchmachine
added
Team:Core/Infra
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Is this sensible? |
|
@thecoop but by that reasoning we'd have to add The logic for writing is simply the same across all collections, but reading for efficiency reasons specifies what kind of collection you want to get out of it. Also, if I want to write a list, how do I decide if I want to writeList or writeCollection, it's weird to have two aliases that always mean the same thing for the same method IMO. Plus it makes profiling more bothersome. |
|
Ah, I hadn't spotted we had |
|
Thanks Simon! |
The `StreamOutput` and `StreamInput` APIs are designed so that code which serializes objects to the transport protocol aligns closely with the corresponding deserialization code. However today `StreamOutput#writeCollection` pairs up with a variety of methods on `StreamInput`, including `readList`, `readSet`, and so on. These methods are not obviously compatible with `writeCollection` unless you look at the implementation, and that makes verifying transport protocol code harder than it needs to be. This commit renames these methods to `readCollectionAsList`, `readCollectionAsSet`, and so on, to clarify that they are compatible with `writeCollection`. Relates elastic#98971 (comment)
|
I think Simon's concern about the naming of these things is valid, it is confusing that the read and write methods don't align any more. But I also agree that it doesn't make sense to have |
The `StreamOutput` and `StreamInput` APIs are designed so that code which serializes objects to the transport protocol aligns closely with the corresponding deserialization code. However today `StreamOutput#writeCollection` pairs up with a variety of methods on `StreamInput`, including `readList`, `readSet`, and so on. These methods are not obviously compatible with `writeCollection` unless you look at the implementation, and that makes verifying transport protocol code harder than it needs to be. This commit renames these methods to `readCollectionAsList`, `readCollectionAsSet`, and so on, to clarify that they are compatible with `writeCollection`. Relates #98971 (comment)
Noticed this when benchmarking FieldCaps transport messages. The
writeListalias just adds more lines to the code and makes profiling more annoying to read, lets remove it.