-
Notifications
You must be signed in to change notification settings - Fork 42
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
Handle FT.PROFILE response (impacted with Coordinator) processing with "ProfilingInformation" #373
Handle FT.PROFILE response (impacted with Coordinator) processing with "ProfilingInformation" #373
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a breaking change, the new structure should be more complete IMO, you could include the raw result as well so you don't need a code change to get to the latest view of the profiler result, but I would at least expect to see the new API to be fully mapped in the response.
|
||
public class ProfilingInformation | ||
{ | ||
public RedisResult Info { get; private set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There doesn't seem to be a lot of value-add here (in fact I think the old structure is cleaner), also - this is a major breaking change, so this really ought to be fleshed out much more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @slorello89, the intention is to avoid client API from throwing an exception due to response layout that Redis server provides across different versions. Considering, for a consumer app, server version would be updated first(most of the time), a fully mapped return type leaves consumer apps with no use of profile command when a breaking change on server side. Another compelling reason to not fully map is that command use cases are debugging purposes mostly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Closes/Fixes #312
FT.PROFILE returns a two-element array reply. The first element contains the results of the provided FT.SEARCH or FT.AGGREGATE command. The second element contains information about query creation, iterator profiles, and result processor profiles.
Profiling details part of response has changed to contain information from shards and coordinator.
Previous and new response layouts given below;
To handle this change and possible future iterations,
ProfilingInformation
introduced as part of return type, which provides a more generic type and leave the choice of how the results are consumed to the developers;Previous response layout
New response layout