Skip to content

[PlacementGroup]Add guarded by in placement group scheduler ut#11306

Merged
rkooo567 merged 17 commits intoray-project:masterfrom
antgroup:add_GUARDED_BY_in_gcs_ut
Oct 20, 2020
Merged

[PlacementGroup]Add guarded by in placement group scheduler ut#11306
rkooo567 merged 17 commits intoray-project:masterfrom
antgroup:add_GUARDED_BY_in_gcs_ut

Conversation

@clay4megtr
Copy link
Copy Markdown
Contributor

@clay4megtr clay4megtr commented Oct 9, 2020 •
edited
Loading

Why are these changes needed?

add GUARDED_BY annotation for gcs scheduler ut.

Related issue number

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 :(

@clay4megtr clay4megtr requested review from ffbin and rkooo567 October 9, 2020 09:26
ffbin
ffbin reviewed Oct 9, 2020
View reviewed changes
ffbin
ffbin reviewed Oct 9, 2020
View reviewed changes
@clay4megtr clay4megtr changed the title Add guarded by in gcs ut [GCS]Add guarded by in gcs ut Oct 9, 2020
@rkooo567
Copy link
Copy Markdown
Contributor

rkooo567 commented Oct 9, 2020

@clay4444. Can you assign me to the "assignee" section instead? That's the formal process we are trying to use.

rkooo567
rkooo567 reviewed Oct 9, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@rkooo567 rkooo567 left a comment

Choose a reason for hiding this comment

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

Can you add GUARDED_BY to all vector attributes?

@rkooo567 rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Oct 9, 2020
@clay4megtr
Copy link
Copy Markdown
Contributor Author

clay4megtr commented Oct 10, 2020

@clay4444. Can you assign me to the "assignee" section instead? That's the formal process we are trying to use.

yes, I get it~ :)

@clay4megtr
Copy link
Copy Markdown
Contributor Author

clay4megtr commented Oct 10, 2020

Can you add GUARDED_BY to all vector attributes?

hi, @rkooo567 I don't quite understand the 'all vector attributes', do you mean all vector need add GUARDED_BY ? in the code, only success_placement_groups_ and failure_placement_groups_ will modified by multi thread.

@rkooo567
Copy link
Copy Markdown
Contributor

rkooo567 commented Oct 12, 2020

@clay4444 Yes! I didn't know the first vector wasn't used by other threads (I just assumed this lock should guard all the vector attributes based on the name "vector_lock"). The other vector is raylet_clients_.

@clay4megtr
Copy link
Copy Markdown
Contributor Author

clay4megtr commented Oct 12, 2020

@clay4444 Yes! I didn't know the first vector wasn't used by other threads (I just assumed this lock should guard all the vector attributes based on the name "vector_lock"). The other vector is raylet_clients_.

yes, the name "vector_lock" is confused indeed, do you think we need change it to "placement_groups_requests_lock_" or others ?

@rkooo567
Copy link
Copy Markdown
Contributor

rkooo567 commented Oct 12, 2020

SGTM :)!

@clay4megtr clay4megtr removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Oct 13, 2020
@clay4megtr
Copy link
Copy Markdown
Contributor Author

clay4megtr commented Oct 13, 2020

Please remove the @author-action-required tag once it is ready to go :)

ok~ 😄

@clay4megtr
Copy link
Copy Markdown
Contributor Author

clay4megtr commented Oct 14, 2020

@ffbin @rkooo567 , is this can be merge ?

@rkooo567
Copy link
Copy Markdown
Contributor

rkooo567 commented Oct 14, 2020

I’ll review it again by today

@clay4megtr
Copy link
Copy Markdown
Contributor Author

clay4megtr commented Oct 14, 2020

I’ll review it again by today

yeah~ thank u :)

ffbin
ffbin reviewed Oct 14, 2020
View reviewed changes
ffbin
ffbin reviewed Oct 14, 2020
View reviewed changes
ffbin
ffbin reviewed Oct 14, 2020
View reviewed changes
ffbin
ffbin reviewed Oct 14, 2020
View reviewed changes
ffbin
ffbin reviewed Oct 14, 2020
View reviewed changes
rkooo567
rkooo567 approved these changes Oct 14, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@rkooo567 rkooo567 left a comment

Choose a reason for hiding this comment

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

I think it is good to go if you address @ffbin's comments. Please remove the tag once you are ready! I will merge them :)

@rkooo567
Copy link
Copy Markdown
Contributor

rkooo567 commented Oct 14, 2020

Please add the test_ok tag if tests look good to go!

@clay4megtr clay4megtr assigned clay4megtr and unassigned ffbin and rkooo567 Oct 14, 2020
@rkooo567 rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Oct 14, 2020
@clay4megtr
Copy link
Copy Markdown
Contributor Author

clay4megtr commented Oct 14, 2020

Please add the test_ok tag if tests look good to go!

ok~

@ffbin ffbin changed the title [GCS]Add guarded by in gcs ut [PlacementGroup]Add guarded by in placement group scheduler ut Oct 14, 2020
@clay4megtr clay4megtr assigned ffbin and rkooo567 and unassigned clay4megtr Oct 15, 2020
@clay4megtr
Copy link
Copy Markdown
Contributor Author

clay4megtr commented Oct 15, 2020

hi, @rkooo567 , I always encounter timeout problems with python ut, the code segment is _wait_for_node method in cluster_utils.py, I think this is not related to the current pr, I have tried hard to fix it, but it will hardly reappear on my machine..... cc @ffbin

@clay4megtr clay4megtr added tests-ok The tagger certifies test failures are unrelated and assumes personal liability. and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Oct 17, 2020
@rkooo567 rkooo567 merged commit 1b3b009 into ray-project:master Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ffbin ffbin ffbin approved these changes

@rkooo567 rkooo567 rkooo567 approved these changes

Assignees

@ffbin ffbin

@rkooo567 rkooo567

Labels

tests-ok The tagger certifies test failures are unrelated and assumes personal liability.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@clay4megtr @rkooo567 @ffbin