Hiredis asio integration #1547
Hiredis asio integration #1547
Conversation
|
Test PASSed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test PASSed. |
|
Test PASSed. |
|
Test PASSed. |
|
Test PASSed. |
|
Test PASSed. |
|
Test PASSed. |
src/ray/gcs/asio.h
Outdated
| bool read_requested_; | ||
| bool write_requested_; | ||
| bool read_in_progress_; | ||
| bool write_in_progress_; |
There was a problem hiding this comment.
Can you document the meaning of these four bools? Also, what are the valid combinations? E.g., is any assignment of True/False to read_requested_/read_in_progress_ valid?
Can we have multiple reads/writes happening at the same time?
src/ray/gcs/asio_test.cc
Outdated
| class TestRedisAsioClient : public ::testing::Test { | ||
| public: | ||
| TestRedisAsioClient() { | ||
| int r = system("redis-server > /dev/null &"); |
There was a problem hiding this comment.
This will in general find a different redis-server from the one we installed, right?
There was a problem hiding this comment.
Yeah, this is only for the test, we don't use modules there.
There was a problem hiding this comment.
It may not run if the user hasn't done sudo apt-get install redis or something like that.
There was a problem hiding this comment.
I added a comment to the test
src/ray/gcs/asio_test.cc
Outdated
| std::cout << "TestRedisAsioClient: redis-server status code was" << r | ||
| << std::endl; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Does the server get shut down? If so, where?
| // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| // KIND, either express or implied. See the License for the | ||
| // specific language governing permissions and limitations | ||
| // under the License. |
There was a problem hiding this comment.
Probably remove this file, is this file copied from somewhere?
src/ray/gcs/asio.cc
Outdated
| void RedisAsioClient::cleanup() {} | ||
|
|
||
| static inline RedisAsioClient *cast_to_client(void *privdata) { | ||
| assert(privdata); |
There was a problem hiding this comment.
When should we use assert and when should we use CHECK? Or does it make sense to stick with one everywhere?
There was a problem hiding this comment.
CHECK_NOTNULL(privdata)
src/ray/gcs/asio.cc
Outdated
| if (!ec || ec == boost::asio::error::would_block) { | ||
| operate(); | ||
| } | ||
| } |
There was a problem hiding this comment.
If we reach this part of the code that means that an error occurred, right? So maybe we should have a check failure if we get here (similarly with the other methods).
The downside is potential verbosity at exit, but maybe we can have the redis clients disconnect before killing the redis server to get around this.
| @@ -81,3 +81,7 @@ if(RAY_BUILD_TESTS OR RAY_BUILD_BENCHMARKS) | |||
|
|
|||
| add_dependencies(gflags gflags_ep) | |||
| endif() | |||
|
|
|||
| set(Boost_USE_STATIC_LIBS ON) | |||
| find_package(Boost COMPONENTS system filesystem REQUIRED) | |||
There was a problem hiding this comment.
We'll probably need to build these ourselves in order to compile with fPIC and link statically (might as well do this on Mac and Ubuntu).
There was a problem hiding this comment.
Right now it is custom compiled on linux and brew installed on mac. We can custom compile on both (in a separate PR).
src/ray/gcs/asio.cc
Outdated
| #include "asio.h" | ||
|
|
||
| RedisAsioClient::RedisAsioClient(boost::asio::io_service &io_service, | ||
| redisAsyncContext *ac) |
There was a problem hiding this comment.
What about replacing ac with async_context or redis_context?
src/ray/gcs/asio.cc
Outdated
| } | ||
| } | ||
|
|
||
| void RedisAsioClient::handle_read(boost::system::error_code ec) { |
There was a problem hiding this comment.
How about ec -> error_code?
src/ray/gcs/asio.cc
Outdated
|
|
||
| void RedisAsioClient::cleanup() {} | ||
|
|
||
| static inline RedisAsioClient *cast_to_client(void *privdata) { |
There was a problem hiding this comment.
How about privdata -> private_data?
|
Test PASSed. |
|
Test PASSed. |
|
Test PASSed. |
What do these changes do?
This integrates hiredis with the asio event loop.