Skip to content
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

Add support for CLIENT TRACKINGINFO #2862

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

tishun
Copy link
Collaborator

@tishun tishun commented May 24, 2024

Closes #2761

Make sure that:

  • You have read the contribution guidelines.
  • You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

@tishun tishun changed the title Topic/tishun/issue 2761 Add support for CLIENT TRACKINGINFO May 24, 2024
@tishun tishun self-assigned this May 24, 2024
@tishun tishun added the type: enhancement A general enhancement label May 24, 2024
@tishun tishun added this to the 7.x milestone May 24, 2024
@tishun tishun marked this pull request as ready for review May 24, 2024 15:36
Copy link
Collaborator

@uglide uglide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should return Map<String, String> to make the response usable even without a parser.

@tishun
Copy link
Collaborator Author

tishun commented May 29, 2024

This is what I initially went with, but the existing CommandOutput abstractions could not handle a Map result with nested Array. Both the MapOutput and the GenericMapOutput could not handle this.

The only place we had a similar parsing done is with the CLUSTER SLOTS and I replicated this approach.

@mp911de do we have a way to handle this or should I create a new CommandOutput?

@mp911de
Copy link
Collaborator

mp911de commented Jun 4, 2024

@mp911de do we have a way to handle this or should I create a new CommandOutput?

Have you tried ObjectOutput? An alternative could be ArrayOutput. If both do not work, then a custom CommandOutput would be the last alternative.

@tishun
Copy link
Collaborator Author

tishun commented Jun 5, 2024

Sooo folks I'd love to hear from you what you think about the latest solution.

I already had a discussion with @uglide on that, but would love to hear any comments.

Obviously it has some downsides, but I personally find it a good compromise between what we have as a solution right now and having an actual mapping to a domain object. The latter would also incur some more heavy duty maintenance on our part.

@mp911de
Copy link
Collaborator

mp911de commented Jun 6, 2024

The new output is pretty complex and its result seems pretty complex to understand as well. A Object or List<Object> return is typically much easier to consume as it would contain Redis primitives (Lists, numbers, strings) that are more common than the approach used here.

@tishun
Copy link
Collaborator Author

tishun commented Jun 6, 2024

The new output is pretty complex and its result seems pretty complex to understand as well. A Object or List<Object> return is typically much easier to consume as it would contain Redis primitives (Lists, numbers, strings) that are more common than the approach used here.

I agree. It is a tradeoff.

The proposed solution is supposed to be used (mainly) in conjunction with the parser logic. Further down the line the parser could be extended to generate stubs based on some schema definition. For a strongly typed language such as Java this is the only working way to consume an API of a service, that has complex types and could be changed down the line.

Jedis has a model object for complex return types, but this makes the design tightly coupled with the current API. Any change to the API would break the existing model.

I was aiming for a middle ground where we have a utility to help the user consume the logic in a simple way (see the integration test), but also allow them to parse the information themselves if the model changes.

A good example for this is if another value is added to the Map. The consumers of the driver could start polling the new value without having to wait until we release a new version of the parser.

So we gat a bit of more stability for the price of having to deal with this interim object that is a bit complex to use.

@tishun
Copy link
Collaborator Author

tishun commented Jun 7, 2024

We should return Map<String, String> to make the response usable even without a parser.

@uglide what is your take on this solution?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for CLIENT TRACKINGINFO
4 participants