[Refactor][GCS] Merge RedisAsioClient into RedisAsyncContext #49000
Conversation
src/ray/gcs/redis_async_context.h
Outdated
| // A write is currently in progress | ||
| bool write_in_progress_{false}; | ||
|
|
||
| void operate(); |
There was a problem hiding this comment.
Any chance for documentation? operate, cleanup, and most of the function names are confusing without inspecting the code, please add some documentations, thanks.
There was a problem hiding this comment.
Acutally I just copy them from the original asio.h. There is also no comment there. But I'll try to add some comments.
src/ray/gcs/redis_async_context.cc
Outdated
| void RedisAsyncContext::operate() { | ||
| if (read_requested_ && !read_in_progress_) { | ||
| read_in_progress_ = true; | ||
| socket_.async_read_some(boost::asio::null_buffers(), |
There was a problem hiding this comment.
I'm curious about the logic:
- From doc, the bytes read is passed to callback
- How do we know all bytes have been read from socket?
- LL145 shows we directly mark ongoing read/write operation as false
There was a problem hiding this comment.
Good question. This might be due to a historical reason. We can ask other team members why we don't use async_read here.
| @@ -92,8 +104,34 @@ class RedisAsyncContext { | |||
| /// should be minimum. | |||
| std::mutex mutex_; | |||
There was a problem hiding this comment.
Actually I'm not quite sure we should use mutex and multi-threading here.
The only operation which might involve multi-threaded operation is operate invoked from another thread, all other operations are posted into io_service; curious why do we need concurrent operate calls?
There was a problem hiding this comment.
Actually, I don't know either. But I suspect that the predecessors decided to use a mutex because they saw this: https://github.com/redis/hiredis/blob/8d8703ee6149ffd3b38cebedd71ad8d1fac63cc7/README.md?plain=1#L152.
There was a problem hiding this comment.
They want to make redisAsyncContext thread-safe.
MortalHappiness
force-pushed
the
feature/merge-redisasioclient-asynccontext
branch
from
bef27a1 to
334dfe6
Compare
|
|
||
| void RedisAsyncContext::DelWrite() { write_requested_ = false; } | ||
|
|
||
| void RedisAsyncContext::Cleanup() { |
There was a problem hiding this comment.
This used to be empty, is it intentional change?
There was a problem hiding this comment.
Referenced from adapters examples in hiredis.
Signed-off-by: Chi-Sheng Liu <[email protected]>
Signed-off-by: Chi-Sheng Liu <[email protected]>
Signed-off-by: Chi-Sheng Liu <[email protected]>
Signed-off-by: Chi-Sheng Liu <[email protected]>
Signed-off-by: Chi-Sheng Liu <[email protected]>
MortalHappiness
force-pushed
the
feature/merge-redisasioclient-asynccontext
branch
from
334dfe6 to
3be9873
Compare
| @@ -32,6 +36,12 @@ typedef void redisCallbackFn(struct redisAsyncContext *, void *, void *); | |||
|
|
|||
| namespace ray { | |||
| namespace gcs { | |||
| // Adaptor callback functions for hiredis redis_async_context->ev | |||
| void CallbackAddRead(void *); | |||
There was a problem hiding this comment.
You don't need to place it in header file...
…ject#49000) Signed-off-by: Chi-Sheng Liu <[email protected]> Signed-off-by: ujjawal-khare <[email protected]>
Why are these changes needed?
While working on #48781, we found that
RedisAsioClientandRedisAsyncContextstrongly depends on each other, and it is a 1-1 mapping. Therefore, we decide to merge the implementation ofRedisAsioClientintoRedisAsyncContextto make it self-contained.The class diagram before this PR:
Note:
redisAsyncContextimplicitly usesRedisAsioClientvia the callback function.The class diagram after this PR:
Related issue number
N/A
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.