Skip to content

Conversation

a-TODO-rov
Copy link
Contributor

@a-TODO-rov a-TODO-rov commented Aug 22, 2025

See #3447

@a-TODO-rov a-TODO-rov requested review from uglide and tishun August 22, 2025 14:28
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.

@tishun let me know if you agree on the API change I suggest

@a-TODO-rov a-TODO-rov force-pushed the CAE-534-cursor-aggregate-v2 branch from cb40503 to e9fa8f5 Compare September 16, 2025 14:51
Add FT.ALTER, FT.DROPINDEX (and DD), FT.SYNDUMP, FT.SYNUPDATE
@a-TODO-rov a-TODO-rov changed the title CAE-534 - RediSearch: Sticky Cursor in Cluster (Stage 1) RediSearch - override cluster behavior Sep 17, 2025
@a-TODO-rov a-TODO-rov linked an issue Sep 17, 2025 that may be closed by this pull request
@a-TODO-rov a-TODO-rov requested review from uglide and ggivo September 17, 2025 11:54
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.

LGTM

Copy link
Collaborator

@tishun tishun left a comment

Choose a reason for hiding this comment

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

So I think that if we

  • (optional) add the commands to the ReadOnlyCommands
  • (required) and we also make sure the right commands are made keyless
    ... we would have the basis to implement the command tips / request-response policies

Why:

  • in the RedisAdvancedCluster* we will override the logic to what we want the driver to do
  • we would have the right API, which is extremely important (no commands that are keyless would be marked like having keys)
  • the right commands would be read-only commands

With a new change we can then do the necessary changes and extract the logic from the RedisAdvancedCluster* elsewhere (and in a single place).

Comment on lines 914 to 915
// --- Upstream-only selection (for write Search commands or no ReadFrom) ---
private RedisClusterNode randomUpstreamNode() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make this method with package visibility and add JavaDoc explaining what it does?

Copy link
Contributor Author

@a-TODO-rov a-TODO-rov Sep 24, 2025

Choose a reason for hiding this comment

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

done
there is a new method now - one that accounts user ReadFrom preferences
check:
6d8dad4

@a-TODO-rov
Copy link
Contributor Author

So I think that if we

  • (optional) add the commands to the ReadOnlyCommands
  • (required) and we also make sure the right commands are made keyless
    ... we would have the basis to implement the command tips / request-response policies

Why:

  • in the RedisAdvancedCluster* we will override the logic to what we want the driver to do
  • we would have the right API, which is extremely important (no commands that are keyless would be marked like having keys)
  • the right commands would be read-only commands

With a new change we can then do the necessary changes and extract the logic from the RedisAdvancedCluster* elsewhere (and in a single place).

  1. I added the read-only RediSearch commands to the default values of ReadOnlyCommands. So now if the user prefers to ReadFrom.REPLICA, those commands will be routed to REPLICAS. This is possible after this commit
  2. Although i agree that changing the API and making the keyless commands truly keyless is nice to have, i can't agree that this is required in the scope of the current PR.
    Why:
  • Has no impact on the current solution - weather we make those changes or not, nothing will change in the way the solution works in this PR. As i understand its more like a technical debt cleanup and preparation for the future (that is unknown)
  • Separation of concerns - i don't like to "pollute" a ready, tested and working solution with unnecessary(for the current scope) changes, that can bring their own bugs.
  • This is supposed to be a step in implementing a more general, centralized solution, but i am not sure in that statement - not until some kind of architecture is drafted and agreed on.

However, from my perspective, those changes will have impact over how the users perceive the API (currently when commands have key, the user expects slotHash routing in cluster). Thats why i will be happy to introduce those changes, but as a part of different PR, if you and @uglide agree.

@a-TODO-rov
Copy link
Contributor Author

You can check the API changes for keyless search commands in #3456

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

Successfully merging this pull request may close these issues.

RediSearch: Client‑side routing & sticky cursors
3 participants