Give NodeInfo a more flexible serialization/deserialization style#54305
Closed
williamrandolph wants to merge 6 commits intoelastic:masterfrom
Closed
Give NodeInfo a more flexible serialization/deserialization style#54305williamrandolph wants to merge 6 commits intoelastic:masterfrom
williamrandolph wants to merge 6 commits intoelastic:masterfrom
Conversation
This is a first cut at giving NodeInfo the ability to carry a flexible list of heterogeneous info responses. The trick is to be able to serialize and deserialize an arbitrary list of blocks of information. It is convenient to be able to deserialize into usable Java objects so that we can aggregate nodes stats for the cluster stats endpoint. In order to provide a little bit of clarity about which objects can and can't be used as info blocks, I've introduced a new interface called "ReportingService."
Collaborator
|
Pinging @elastic/es-core-infra (:Core/Infra/Plugins) |
Contributor
Author
|
One option I have here is that I could use the map of "ReportingService.Info" objects and see how well it works as the refactoring proceeds, but hold off on changing the serialization for as long as possible, i.e., until we actually start plugging in other reporting services. The more I think about this, the more I like the idea. |
Contributor
Author
|
Yeah, I'm going to punt on changing the response serialization until I'm more sure of what I want it to look like. I'll issue a new PR with the part of this work that doesn't affect serialization. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In #53140, we changed the
NodesInfoRequestclass so that its serialization could handle requests for arbitrary lists of metrics rather than a hard-coded group. Now we need to do something similar for the response.NodesInfoResponsejust rolls up a list ofNodeInfoobjects, so this work really needs to happen inNodeInfo.There are two features of the
NodeInfoclass that have made this a little trickyClusterStatsNodes, access some of the info objects in order to aggregate information from different nodes.At this stage of the refactoring, it doesn't seem possible to treat the blocks of info as maps of strings. It seems that we still need to be able to deserialize into objects.
For my first approach, I've tried to leverage the existing
NamedWriteablecode to read the info objects out of the stream. I've created aReportingServiceinterface and aReportingService.Infointerface in order to mark the classes that can be used as info blocks; perhaps this is overkill at this stage. I've also used a map keyed on classes in order to handle all casting within this class. Instead of callingOsInfo os = nodeInfo.getOs(), we can now callOsInfo os = nodeInfo.getInfo(OsInfo.class).We have talked about, in the long run, serializing and deserializing these blocks of info as
Map<String, Map<String, String>>types. If we do that, it seems like we might have to take a significantly different approach to calculating cluster stats. At the moment, cluster stats uses NodesInfo and NodesStats requests as a way to bundle the various info and stats objects it needs to aggregate. I am still thinking through how to get there from here.I don't want to merge this PR unless we're sure it's the right way forward, but I would like to get some feedback on it.