Adds Sentinel support #345
Conversation
|
Still an early prototype, I want to check that my CI approach is viable. |
anarthal
force-pushed
the
feature/sentinel
branch
from
ae013c1 to
a228511
Compare
|
|
||
| // Store the resulting address in a well-known place | ||
| if (st.cfg.sentinel.server_role == role::master) { | ||
| st.cfg.addr = resp.master_addr; |
There was a problem hiding this comment.
IMO this assignment should be moved to the run_op right after async_sentinel_resolve resumes to make that function more readable. It would be more intuitive if async_sentinel_resolve complete with the value instead of changing the connection_impl state. Can we change the completion signature to void(error_code, address) to achieve that?
EDIT: We could also extend the completion signature with a third parameter which are the new sentinel addresses which is also use in run_op. This way the async_sentinel_resolve would not change any state in connection_impl.
There was a problem hiding this comment.
Moving the assignment or the update_sentinel_list to run makes run (which is already big) have even more responsibilities. The tests are already gigantic, so I'm not very keen on this.
I don't think adding a 2nd (or 3rd) completion parameter to the run FSM makes a lot of good, because the parameter needs to be there for code paths, but has a valid value only in one of it. Unless you have a very clear opinion on this, I'd prefer for this to stay as is.
| } | ||
|
|
||
| // Parses a list of replicas or sentinels | ||
| inline system::error_code parse_server_list( |
There was a problem hiding this comment.
- Can we move this to .ipp?
- Can we take a
std::vector<node>instead and work with.at()function instead of iterators. We don't need performance here so I would prefer to be on the safer side.
There was a problem hiding this comment.
Can we move this to .ipp?
I don't think it makes sense because this is an impl/ file. It gets included by .ipp files only, and is never seen by the end-user code.
There was a problem hiding this comment.
Can we take a std::vector instead and work with .at() function instead of iterators. We don't need performance here so I would prefer to be on the safer side.
I'm gonna defer this to #355. flat_tree contains the number of messages, and this makes parsing much easier. None of these asserts can be triggered unless there's a bug in the parser, so it's not much of a problem. I will address it though, since you're right about the security/performance tradeoff here.
|
I am half way through it. The sentinel_utils.hpp implements hard stuff so I will have a more detailed look at it, perhaps tomorrow. |
|
I finished reviewing. I haven't found anything serious to fix only things to consider. We can also do it in another PR to avoid delaying a merge, we have plenty of time until the next release. Many thanks you implement this pretty fast considering its size. |
|
One last question, IIUC we resolve with the sentinel using the same |
|
When merged we can also close these tickets https://github.com/boostorg/redis/issues?q=is%3Aissue%20state%3Aopen%20label%3Asentinel |
This reverts commit 44c7f4a.
anarthal
left a comment
anarthal
left a comment
There was a problem hiding this comment.
One last question, IIUC we resolve with the sentinel using the same connection_impl that we use to interact with the server. Is it safe for users to star async_exec, receive and cancellation on these ops while the master/replica is being resolved? Shouldn't we add a new connection_impl that is only used to resolve with sentinel? Or is it clear to you that mixing these ops won't interfere with each other?
It's safe. Actually, the exec test hits this situation, and I've added a receive test this for receive. exec only touches the multiplexer queue, and receive2 the channel. Everything else is managed by run. run ensures that you're either resolving addresses or communicating with the server, but not both.
We could extract the read buffer from the multiplexer to make this point even clearer, since it's used by async_exec_one, and the current usage is a little bit weird. But I'll probably defer this to another PR.
2 participants
close #237
close #269
close #268
close #229