Skip to content

[xray] Fix GCS table prefixes #2065

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
stephanie-wang merged 2 commits into from
May 16, 2018
Merged

[xray] Fix GCS table prefixes #2065

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb4a92a
Fix GCS table prefixes
stephanie-wang May 15, 2018
ba31818
More explicit documentation
stephanie-wang May 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions src/ray/gcs/tables.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,11 @@ class PubsubInterface {

/// \class Log
///
/// A GCS table where every entry is an append-only log.
/// A GCS table where every entry is an append-only log. This class is not
/// meant to be used directly. All log classes should derive from this class
/// and override the prefix_ member with a unique prefix for that log, and the
/// pubsub_channel_ member if pubsub is required.
///
/// Example tables backed by Log:
/// ObjectTable: Stores a log of which clients have added or evicted an
/// object.
Expand Down Expand Up @@ -171,10 +175,12 @@ class Log : virtual public PubsubInterface<ID> {
/// The GCS client.
AsyncGcsClient *client_;
/// The pubsub channel to subscribe to for notifications about keys in this
/// table. If no notifications are required, this may be set to
/// TablePubsub_NO_PUBLISH.
/// table. If no notifications are required, this should be set to
/// TablePubsub_NO_PUBLISH. If notifications are required, then this must be
/// unique across all instances of Log.
TablePubsub pubsub_channel_;
/// The prefix to use for keys in this table.
/// The prefix to use for keys in this table. This must be unique across all
/// instances of Log.
TablePrefix prefix_;
/// The index in the RedisCallbackManager for the callback that is called
/// when we receive notifications. This is >= 0 iff we have subscribed to the
Expand All @@ -194,7 +200,11 @@ class TableInterface {

/// \class Table
///
/// A GCS table where every entry is a single data item.
/// A GCS table where every entry is a single data item. This class is not
/// meant to be used directly. All table classes should derive from this class
/// and override the prefix_ member with a unique prefix for that table, and
/// the pubsub_channel_ member if pubsub is required.
///
/// Example tables backed by Log:
/// TaskTable: Stores Task metadata needed for executing the task.
template <typename ID, typename Data>
Expand Down Expand Up @@ -288,7 +298,7 @@ class ActorTable : public Log<ActorID, ActorTableData> {
ActorTable(const std::shared_ptr<RedisContext> &context, AsyncGcsClient *client)
: Log(context, client) {
pubsub_channel_ = TablePubsub_ACTOR;
prefix_ = TablePrefix_TASK_RECONSTRUCTION;
prefix_ = TablePrefix_ACTOR;
}
};

Expand All @@ -297,8 +307,7 @@ class TaskReconstructionLog : public Log<TaskID, TaskReconstructionData> {
TaskReconstructionLog(const std::shared_ptr<RedisContext> &context,
AsyncGcsClient *client)
: Log(context, client) {
pubsub_channel_ = TablePubsub_ACTOR;
Copy link
Contributor

@concretevitamin concretevitamin May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for pubsub_channel_ = TablePubsub_TASK_RECONSTRUCTION;?

Copy link
Contributor Author

@stephanie-wang stephanie-wang May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, because pubsub isn't enabled for this table. There's a comment about this for the pubsub_channel_ declaration in the base class, but it's a bit hidden...I'll add a note to the class documentation.

prefix_ = TablePrefix_ACTOR;
prefix_ = TablePrefix_TASK_RECONSTRUCTION;
}
};

Expand Down