Skip to content

Conversation

@anarthal
Copy link
Collaborator

@anarthal anarthal commented Sep 11, 2025

Adds config::setup and config::use_setup, to run arbitrary Redis commands on connection establishment
Improves docs for config::{username, password, clientname, database_index}
Splits all connection establishment tests into test_conn_hello

close #302

@anarthal anarthal requested a review from mzimbres September 11, 2025 18:12
@anarthal
Copy link
Collaborator Author

Some notes

  • detail::request_access is currently used to workaround the restriction that only HELLO requests might have their priority set. It's used to expose a method outside request, but maintaining it private to the library. My intention is deprecating request::config::hello_with_priority and making it something internal. You proposed a nice value, but I think it's prone to misuse, and it'd impose extra overhead by having to order every request in the queue.
  • I haven't deprecated any config members yet because using config::setup is not straightforward without the command wrappers, and it'd require examples on how to do it now, at least.

@mzimbres
Copy link
Collaborator

Thanks. We could perhaps also remove this that sets priority conditionally only if is a HELLO command. Looks like an unnecessary complication.

@mzimbres
Copy link
Collaborator

You proposed a nice value, but I think it's prone to misuse, and it'd impose extra overhead by having to order every request in the queue.

We already have O(n) from the std::stable_partition so sorting would only add an extra Olog(n)) to the complexity. But I agree there isn't any strong usecase for it so far.

@anarthal
Copy link
Collaborator Author

Thanks. We could perhaps also remove this that sets priority conditionally only if is a HELLO command. Looks like an unnecessary complication.

Removing the check breaks everything because the priority flag defaults to true in the constructor. Changing it to false is technically a breaking change. In the next PR I will deprecate the priority functions and remove all this in a subsequent release.

@anarthal
Copy link
Collaborator Author

anarthal commented Sep 14, 2025

We already have O(n) from the std::stable_partition so sorting would only add an extra Olog(n)) to the complexity. But I agree there isn't any strong usecase for it so far.

I don't know to which extent these O(n) and O(nlogn) are comparable. stable_partition is called in:

  • cancel_on_conn_lost: called once per disconnection. Never during regular operation. We can likely neglect this cost.
  • cancel_waiting: similar argument to the above.
  • release_push_requests: called once per write. There is no limit to the number of requests a write may have.

On the other hand, sorting price gets paid once per added request, rather than once per write. So there's a factor that this analysis is not considering.

I think the multiplexer can be modified to use linked lists instead of a vector, and that would yield better efficiency. But I'd need to write the code and benchmark it to be sure, so I haven't done anything yet. In any case, I wouldn't pay this price, at least until some user asks for it.

@anarthal
Copy link
Collaborator Author

I think I've addressed all your comments. I've also done a general renaming removing "hello" and "handshake" in favor of "setup request".

namespace boost::redis::detail {

void setup_hello_request(config const& cfg, request& req)
void setup_hello_request(config& cfg, request& req)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be renamed to set_request_priority.

@mzimbres
Copy link
Collaborator

Thanks.

@anarthal anarthal merged commit 6a1a07f into boostorg:develop Sep 15, 2025
17 checks passed
@anarthal anarthal deleted the feature/302-optional-hello branch September 15, 2025 12:19
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.

Make command HELLO optional

2 participants