ARROW-2551: [Plasma] Improve notification logic #2031
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2031 +/- ##
==========================================
+ Coverage 86.35% 86.37% +0.02%
==========================================
Files 242 230 -12
Lines 41184 40474 -710
==========================================
- Hits 35565 34960 -605
+ Misses 5619 5514 -105
Continue to review full report at Codecov.
|
cpp/src/plasma/store.cc
Outdated
| @@ -104,7 +104,7 @@ GetRequest::GetRequest(Client* client, const std::vector<ObjectID>& object_ids) | |||
| num_objects_to_wait_for = unique_ids.size(); | |||
| } | |||
|
|
|||
| Client::Client(int fd) : fd(fd) {} | |||
| Client::Client(int fd) : fd(fd), notification_fd(boost::none) {} | |||
There was a problem hiding this comment.
Let's make notification_fd = -1 by default if not set (and get rid of the boost dependency)
There was a problem hiding this comment.
This looks good to me! Can you get rid of the boost::option dependency and use the value -1 instead of boost::none? At the moment plasma doesn't depend on boost and I'd like to keep it that way unless there is a good reason to introduce it.
Also can you rebase the PR? There is a small conflict at the moment.
zhijunfu
force-pushed
the
refactor-notification
branch
from
38d992b
to
51b292e
Compare
|
@pcmoritz Thanks for reviewing :) Changed boost::none to -1 and did a rebase to resolve the conflict. |
|
@zhijunfu Sorry, the other PR caused a merge conflict, can you rebase/merge so we can get this PR in? Thanks :) |
zhijunfu
force-pushed
the
refactor-notification
branch
from
3199eea
to
f2377f8
Compare
|
@pcmoritz Sorry for the long delay in reply, I just rebased the change on top of latest code. The build failures w/ parquet and tool-chain should be unrelated with the change. |
|
|
||
| /// The file descriptor used to push notifications to client. This is only valid | ||
| /// if client subscribes to plasma store. -1 indicates invalid. | ||
| int notification_fd; |
There was a problem hiding this comment.
Unrelated to the content of this patch: could we regularize the variable name convention in this header and elsewhere in Plasma? Would help with readability
There was a problem hiding this comment.
Yes that makes a lot of sense:)
|
Re-triggered the build and it passed this time. |
There was a problem hiding this comment.
+1. Merging this, and also opened https://issues.apache.org/jira/browse/ARROW-2690
This change targets to improve a few places in current plasma notification code: