[Pubsub] Pubsub module command batch part 1 #16167

rkooo567 · 2021-06-01T00:20:14Z

Why are these changes needed?

Please check this issue for more details; #16048. This implements the pubsub command batch to improve throughput + solve ordering issues. The proposal is at https://docs.google.com/document/d/1OnbmqiXIuyOfJwv_uEShTKrVT9t_vuxNXjX1dc02GW4/edit#heading=h.gi8hbyr0xqg7.

Related issue number

#16048

Checks

I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

rkooo567 · 2021-06-01T01:30:20Z

src/ray/core_worker/reference_count.cc

        object_status_subscriber_->Subscribe(
-            rpc::ChannelType::WORKER_REF_REMOVED_CHANNEL, addr.ToProto(),
-            object_id.Binary(), message_published_callback, publisher_failed_callback);
+            std::move(sub_message), rpc::ChannelType::WORKER_REF_REMOVED_CHANNEL,


This currently is doing nothing. This PR will only implement the command batch in the cpp layer, but the real code actually doesn't use it.

rkooo567 · 2021-06-01T01:30:51Z

src/ray/protobuf/pubsub.proto

+/// Subscribe
+///
+message Command {
+  ChannelType channel_type = 1;


probably unnecessary as Yi pointed out before. I might remove it in the future PRs

src/ray/protobuf/pubsub.proto

src/ray/pubsub/subscriber.h

fishbone · 2021-06-02T23:31:42Z

src/ray/pubsub/subscriber.cc

+  // Update the command in the FIFO order.
+  int64_t updated_commands = 0;
+  while (!commands_.empty() && updated_commands < command_max_batch_size_) {
+    auto &command = commands_.front();
+    auto *new_command = long_polling_request.add_commands();
+    new_command->Swap(command.get());
+    commands_.pop();
+    updated_commands += 1;
+  }


I have some questions here. What do we do for the rest of the commands_? Do you mean that we'll keep calling this function until there is nothing left?

They will be delivered in the next long polling request. But actually, that's a good point. We should reply right away if there are commands left, so that this will be consumed without waiting. I will fix that

To be clear, here's the workflow;

Subscriber batches commands in a long polling "request" -> publisher replies right away although there are no pub messages -> in this way, the subscriber keeps sending long polling "request" until all commands are served.

I can probably add this comment here.

Actually, this will be handled in the next PR where we actually migrate all subscribe to command based subscription.

fishbone · 2021-06-02T23:36:21Z

Could you add some comment about what's the definition of command here? Is this for sub/unsub only? And are the messages going through a separate path?

rkooo567 · 2021-06-02T23:41:25Z

@iycheng Sure. I will update the comment. Here command is from Redis pubsub terminologies.

fishbone · 2021-06-03T06:36:11Z

@iycheng Sure. I will update the comment. Here command is from Redis pubsub terminologies.

Got it. Sorry I'm not very familiar with redis pubsub so I think I lost here.

rkooo567 · 2021-06-03T06:46:28Z

What are terminologies from other systems? Is it just Sub/unsub messages?

clarkzinzow

LGTM, but I'd wait for a final pass by @iycheng before merging to confirm that his feedback was fully addressed.

src/ray/common/ray_config_def.h

src/ray/protobuf/pubsub.proto

src/ray/pubsub/subscriber.h

clarkzinzow · 2021-06-07T21:00:45Z

src/ray/protobuf/pubsub.proto

+  // ObjectID that contains object_id. This is used when an ObjectID is stored
+  // inside another object ID that we do not own. Then, we must notify the
+  // outer ID's owner that the ID contains object_id.


Nit:

Suggested change

// ObjectID that contains object_id. This is used when an ObjectID is stored

// inside another object ID that we do not own. Then, we must notify the

// outer ID's owner that the ID contains object_id.

// ID of object that contains `reference`. This is used when an ObjectID is stored

// inside another object that we do not own. Then, we must notify the

// outer object's owner that the object contains `reference`.

clarkzinzow · 2021-06-07T21:11:59Z

src/ray/pubsub/test/subscriber_test.cc

+  // No more request after that.
+  ASSERT_EQ(owner_client->PopRequest(), nullptr);
+}
+


Nice tests!

fishbone · 2021-06-08T05:08:12Z

Feel good about this one. But please update the shared ptr to unique ptr (or you can try &command as well).
I feel (not confident) all the objects from grpc are managed by mem arena inside grpc. So even it's a struct, I feel the data beneath it is still allocated in the pool, so swap still works in this case.

rkooo567 · 2021-06-09T07:27:01Z

local_object_manager_test is flaky in the master and the first mac build occasionally failed in Travis. This PR doesn't effectively change any external behavior, so it should be safe. Merging it.

rkooo567 added 2 commits May 31, 2021 17:02

Basic command batch implemented.

b0a75c0

working.

025c3f9

rkooo567 commented Jun 1, 2021

View reviewed changes

fix bugs.

12bc8c0

rkooo567 changed the title ~~[WIP] Pubsub module command batch part 1~~ [Pubsub] Pubsub module command batch part 1 Jun 1, 2021

rkooo567 mentioned this pull request Jun 1, 2021

[Pubsub] Ownership based object directory needs to batch requests #16048

Closed

rkooo567 assigned clarkzinzow and fishbone Jun 1, 2021

Improve a protobuf message.

6d0d65e

fishbone reviewed Jun 2, 2021

View reviewed changes

src/ray/protobuf/pubsub.proto Show resolved Hide resolved

fishbone reviewed Jun 2, 2021

View reviewed changes

src/ray/pubsub/subscriber.h Outdated Show resolved Hide resolved

fishbone reviewed Jun 2, 2021

View reviewed changes

fishbone added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jun 3, 2021

rkooo567 added 2 commits June 6, 2021 16:31

Merge branch 'master' into obod-pubsub

dd76ef0

Update description of protobuf.

9e3519c

rkooo567 removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jun 7, 2021

rkooo567 requested a review from fishbone June 7, 2021 03:49

clarkzinzow approved these changes Jun 7, 2021

View reviewed changes

rkooo567 added 4 commits June 7, 2021 22:59

Addressed code review.

495859e

Merge branch 'master' into obod-pubsub

21522de

Merge branch 'master' into obod-pubsub

40a2909

Merge branch 'master' into obod-pubsub

f71c04d

rkooo567 merged commit d9227d8 into ray-project:master Jun 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Pubsub] Pubsub module command batch part 1 #16167

[Pubsub] Pubsub module command batch part 1 #16167

rkooo567 commented Jun 1, 2021 •

edited

Loading

rkooo567 Jun 1, 2021

rkooo567 Jun 1, 2021

fishbone Jun 2, 2021

fishbone Jun 2, 2021

rkooo567 Jun 2, 2021 •

edited

Loading

rkooo567 Jun 6, 2021

rkooo567 Jun 7, 2021

fishbone commented Jun 2, 2021

rkooo567 commented Jun 2, 2021

fishbone commented Jun 3, 2021

rkooo567 commented Jun 3, 2021

clarkzinzow left a comment

clarkzinzow Jun 7, 2021

clarkzinzow Jun 7, 2021

fishbone commented Jun 8, 2021

rkooo567 commented Jun 9, 2021

[Pubsub] Pubsub module command batch part 1 #16167

[Pubsub] Pubsub module command batch part 1 #16167

Conversation

rkooo567 commented Jun 1, 2021 • edited Loading

Why are these changes needed?

Related issue number

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkooo567 Jun 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishbone commented Jun 2, 2021

rkooo567 commented Jun 2, 2021

fishbone commented Jun 3, 2021

rkooo567 commented Jun 3, 2021

clarkzinzow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishbone commented Jun 8, 2021

rkooo567 commented Jun 9, 2021

rkooo567 commented Jun 1, 2021 •

edited

Loading

rkooo567 Jun 2, 2021 •

edited

Loading