Markstream to enable 1-producer-1-consumer on server side #138
Conversation
There was a problem hiding this comment.
I prefer to rename MarkStream as OpenStream, add define an enumeration
enum class OpenStreamMode {
read = 1,
write = 2,
}in a newly added stream_utils.h or somewhere like that, then replace magic 1 and 2 in OpenReader/OpenWriter with the enum, for better code readability and maintain-ability.
| return std::unique_ptr<ByteStreamReader>( | ||
| new ByteStreamReader(client, id_, meta_)); | ||
| } | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
I suggest to have API like
Status OpenReader(Client &client, std::unique_ptr<ByteStreamReader> &r) {
RETURN_ON_ERROR(client.MarkStream(id_, 2))....
r = ...
return Status::OK();
}which will be more consistent will the style of our other APIs, and more importantly, let user know why the operation fail. Return nullptr sliently usually the root of many unintended bugs.
src/server/memory/stream_store.h
Outdated
| @@ -86,6 +88,7 @@ class StreamStore { | |||
| std::shared_ptr<BulkStore> store_; | |||
| size_t threshold_; | |||
| std::unordered_map<ObjectID, std::shared_ptr<StreamHolder>> streams_; | |||
| std::unordered_map<ObjectID, int64_t> stream_marks_; | |||
There was a problem hiding this comment.
Could we use the mark as an attribute of StreamHolder? without introduce an extra map. Considering that when deleting a stream, we need an extra deletion and may lead to inconsistency.
test/stream_test.cc
Outdated
| @@ -83,6 +86,9 @@ int main(int argc, char** argv) { | |||
| CHECK(byte_stream != nullptr); | |||
|
|
|||
| auto writer = byte_stream->OpenWriter(writer_client); | |||
| auto failed_writer = byte_stream->OpenWriter(writer_client); | |||
| CHECK(failed_writer == nullptr); | |||
There was a problem hiding this comment.
For the check, you may also need to consider all places the OpenReader/OpenWriter are called, not merely the test case.
|
LGTM. |
What do these changes do?
Related issue number
Fixes #133