[WIP] Separate serialization logic from class

[VachaShah]

Description

This is a draft of the idea to separate native serialization from classes as discussed in #13178. Currently, it is not possible to decouple Writeable and its implementations from the native protocol classes (without breaking changes) but we can move serialization primitives to their own serializers as a starting point. This will be the building block for adding protobuf in a way that serialization is not coupled with the classes.

For protobuf (or any other future protocol), the idea is not have any serialization within the class but rather it be handled by its own serializer implementation (fe ProtobufQuerySearchResultSerializer) and not tie the class to serde implementations.

Resolves #[Issue number to be closed when this PR is merged]

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• API changes companion pull request created.
• Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
• Commits are signed per the DCO using --signoff
• Commit changes are listed out in CHANGELOG.md file (See: Changelog)
• Public documentation issue/PR created

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[VachaShah]

@dblock @reta Let me know how you feel about this architecture!

[github-actions]

❌ Gradle check result for a8da31f: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[dblock]

Extracting the serializer seems like a good idea on its own merit because then it can be tested separately, etc, however the real problem is that SearchQueryResult should be a generic thing that doesn't implement any specific serialization. If you refactor towards that state, then you can then have a native/SearchQueryResult and protobuf/SearchQueryResult that have each their own serialize and deserialize. AFAIK there's nothing else that needs to be put into those native vs. protobuf versions, so maybe there's no need for a SearchQueryResultNativeSerializer.

[reta]

💯 what @dblock says

however the real problem is that SearchQueryResult should be a generic thing that doesn't implement any specific serialization.

Just to explain why it is a key - the model (classes like SearchQueryResult) will never be aware of the protocol (native / protobuf) so making any serde decisions at that level will not be possible.

[VachaShah]

Thank you @dblock and @reta! Just so I understand correctly, if we do something like native/FetchSearchResult and protobuf/FetchSearchResult, then the serde methods exist into those classes and they come from a common interface of FetchSearchResult. I am wondering though at what point do we decide whether to use native/FetchSearchResult or protobuf/FetchSearchResult in the process 🤔

[reta]

I am wondering though at what point do we decide whether to use native/FetchSearchResult or protobuf/FetchSearchResult in the process 🤔

I think that FetchSearchResult is not impacted anyhow by native / protobuf / ... protocol, this is model class. However, what is impacted is serde for FetchSearchResult, so we will have separate native/FetchSearchResultSerializer or protobuf/FetchSearchResultSerializer in the process. As an example, the method FetchSearchResult(StreamInput in) would be replaced by something like native/FetchSearchResultSerializer::deserialize (and similar for protobuf), which are protocol dependent.

[VachaShah]

I am wondering though at what point do we decide whether to use native/FetchSearchResult or protobuf/FetchSearchResult in the process 🤔

I think that FetchSearchResult is not impacted anyhow by native / protobuf / ... protocol, this is model class. However, what is impacted is serde for FetchSearchResult, so we will have separate native/FetchSearchResultSerializer or protobuf/FetchSearchResultSerializer in the process. As an example, the method FetchSearchResult(StreamInput in) would be replaced by something like native/FetchSearchResultSerializer::deserialize (and similar for protobuf), which are protocol dependent.

+1 to this. So thats what I have drafted in this PR, I cannot remove the FetchSearchResult(StreamInput in) constructor though since its a public class. But for protobuf, we will make sure to not include serde in FetchSearchResult. Does the change in this PR (its just for 2 classes, I will do it for other ones that I touched once we finalize the idea) then look good according to the design?

The purpose of this PR was to move serde out of these classes with a native/*Serializer with a serializer interface on which protobuf can then be implemented. Let me know if that makes sense!

[reta]

+1 to this. So thats what I have drafted in this PR, I cannot remove the FetchSearchResult(StreamInput in) constructor though since its a public class.

Yes, you don't need to, I would say the class itself could be kept as-is (as you mentioned, this is public class, we are constrained here). However, the calls to FetchSearchResult(StreamInput in) should be wired through native/FetchSearchResultSerializer. This is where it matters I think, and for protobuf/FetchSearchResultSerializer the implementation will be entirely inside there, not in FetchSearchResult

[VachaShah]

+1 to this. So thats what I have drafted in this PR, I cannot remove the FetchSearchResult(StreamInput in) constructor though since its a public class.

Yes, you don't need to, I would say the class itself could be kept as-is (as you mentioned, this is public class, we are constrained here). However, the calls to FetchSearchResult(StreamInput in) should be wired through native/FetchSearchResultSerializer. This is where it matters I think, and for protobuf/FetchSearchResultSerializer the implementation will be entirely inside there, not in FetchSearchResult

Ah so right now the calls go from FetchSearchResult(StreamInput in) to native/FetchSearchResultSerializer and a caller calls FetchSearchResult::new to call the constructor. This call will ultimately go to native/FetchSearchResultSerializer from the constructor. What you are saying is a caller should not be calling FetchSearchResult::new with StreamInput but rather calling the serializer itself. Let me know if my understanding is correct!

[reta]

. Let me know if my understanding is correct!

Totally correct, and the protobuf serde would be implemented completely inside the protobuf/FetchSearchResultSerializer class.

[opensearch-trigger-bot]

This PR is stalled because it has been open for 30 days with no activity.

[VachaShah]

Closing this PR since won't be working on this actively. If this change needs to be picked up, please feel free to use my branch https://github.com/VachaShah/OpenSearch/tree/separate-serializer-from-class.