Skip to content
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

Jump to bottom

[Pubsub] Use a pubsub module for Ownership based object directory #16407

Merged
merged 23 commits into from
Jun 16, 2021
Merged

[Pubsub] Use a pubsub module for Ownership based object directory #16407

merged 23 commits into from
Jun 16, 2021

Conversation

rkooo567
Copy link
Contributor

@rkooo567 rkooo567 commented Jun 14, 2021 •
edited
Loading

Why are these changes needed?

This is the last PR to use the pubsub module for ownership based object directory.

NOTE: many of parts are simple refactoring like the previous PRs.

This also introduces a new API for the publisher, PublishFailure which is used to publish the information that the specific key id is failed in the publisher side (so the subscriber invokes the failure handler). If you think this needs to be done just within regular Publish API, lmk

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
rkooo567 commented Jun 14, 2021
View reviewed changes
src/ray/object_manager/ownership_based_object_directory.cc
request,
std::bind(&OwnershipBasedObjectDirectory::SubscriptionCallback, this, object_id,
worker_id, std::placeholders::_1, std::placeholders::_2));
UpdateObjectLocations(object_location_message, object_id, gcs_client_,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

subscription callback doesn't need to pass the status to UpdateObjectLocations anymore because it is only called when the publisher publishes a normal message now.

src/ray/core_worker/core_worker.cc
}
}

void CoreWorker::HandleGetObjectLocationsOwner(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This handler is simplified because we don't need the previous structure to be compatible to both subscription + regular lookup.

fishbone
fishbone reviewed Jun 15, 2021
View reviewed changes
Copy link
Contributor

@fishbone fishbone left a comment

Choose a reason for hiding this comment

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

Mostly looks good. I realized that in the end, lots of the line changes are due to indent only. 😀
Btw, thanks for some improvement of the code in this PR. I have left some comments.

@fishbone 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 15, 2021
clarkzinzow
clarkzinzow approved these changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@clarkzinzow clarkzinzow left a comment

Choose a reason for hiding this comment

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

LGTM as long as @iycheng gives the green light that his feedback has been addressed!

src/ray/core_worker/reference_count.cc Outdated Show resolved Hide resolved
src/ray/object_manager/ownership_based_object_directory.cc Outdated Show resolved Hide resolved
@rkooo567 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 16, 2021
@rkooo567
Copy link
Contributor Author

rkooo567 commented Jun 16, 2021 •
edited
Loading

test_array is originally flaky in the master on Windows. Merging it.

@rkooo567 rkooo567 merged commit 90599d3 into ray-project:master Jun 16, 2021
amogkam added a commit that referenced this pull request Jun 16, 2021
@amogkam
Revert "[Pubsub] Use a pubsub module for Ownership based object direc…
bd869b4 
…tory (#16407)"

This reverts commit 90599d3.
@amogkam amogkam mentioned this pull request Jun 16, 2021
Merged
wuisawesome pushed a commit that referenced this pull request Jun 16, 2021
@amogkam
Revert "[Pubsub] Use a pubsub module for Ownership based object direc…
b986938 
…tory (#16407)" (#16486)

This reverts commit 90599d3.
rkooo567 added a commit to rkooo567/ray that referenced this pull request Jun 16, 2021
@rkooo567
Revert "Revert "[Pubsub] Use a pubsub module for Ownership based obje…
ab8b025 
…ct directory (ray-project#16407)" (ray-project#16486)"

This reverts commit b986938.
@fishbone fishbone mentioned this pull request Jun 19, 2021
Closed
rkooo567 added a commit that referenced this pull request Jun 29, 2021
@rkooo567 @stephanie-wang
Revert revert OBOD pubsub PR (#16487)
804a867 
* Revert "Revert "[Pubsub] Use a pubsub module for Ownership based object directory (#16407)" (#16486)"

This reverts commit b986938.

* revert the obod problem.

* Add stats.

* Fix a possible regression.

* in another progress

* debugging

* Fix stats bug

* update

* Add more stats.

* Add stats

* lint

* Fix issue

* remove spammy logs

* lint

* better error msg for debugging

* Add even more logging

* Remove spammy logs

* Fix iterator invalidation issue

* more debugging info

* fix

* Add more debug logs

* add debug logs

* Remove the problematic line for confirmation

* Completed

* Fixed a broken test.

* experiment

* Lint

* Add a better error message

* try out

* revert the build file.

* In progress again

* IP

* Formatting

* Revert the log level

* Unskip test array

* final clean up.

* fix a build issue

* debug logs

* remove

* .

* Add more critical logs.

* format

* tmp

* log

* log

* issue fix

* Upgrade

* test experiment

* Fix an issue

* Fix issues.

* Lint

* remove unnecessary code

* last clean up.

Co-authored-by: Stephanie Wang <swang@cs.berkeley.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fishbone fishbone fishbone left review comments

@clarkzinzow clarkzinzow clarkzinzow approved these changes

Assignees

@clarkzinzow clarkzinzow

@fishbone fishbone

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rkooo567 @clarkzinzow @fishbone