[Placement Group] Make the creation of placement group sync #13858

clay4444 · 2021-02-02T15:17:50Z

Why are these changes needed?

Make the creation of placement group sync, prevent the core worker returning that the creation was successful but the actual creation failed.
fix ut test_automatic_cleanup_detached_actors

Related issue number

#13859

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

If we'd like to have this change, we should make sure the HandleCreatePlacementGroup will not have this code;

pending_register_iter->second = std::move(callback);

I am not sure if this is necessary.

clay4444 · 2021-02-03T02:00:34Z

If we'd like to have this change, we should make sure the HandleCreatePlacementGroup will not have this code;
pending_register_iter->second = std::move(callback);
I am not sure if this is necessary.

I think this is necessary, image this case: CoreWorker return success, but its creation is actually failed, then the subsequent scheduling of task or actor with PG is meaningless, and if a failover occurs, the placement group info will be lost forever

rkooo567 · 2021-02-03T04:48:24Z

That's why we have .wait and .ready right? Also again, if we'd like to make this change, we shouldn't store the callback for CreatePlacementGroup. We should always reply right away like actor cases. Otherwise, there's no meaning of .wait and .ready APIs.

clay4444 · 2021-02-03T06:15:30Z

That's why we have .wait and .ready right? Also again, if we'd like to make this change, we shouldn't store the callback for CreatePlacementGroup. We should always reply right away like actor cases. Otherwise, there's no meaning of .wait and .ready APIs.

I think the .wait and .ready are for detecting whether create successful instead of registration successful right? if the register is already failed, I think the pg.wait() and pg.ready() Is meaningless

rkooo567 · 2021-02-03T06:18:36Z

if we'd like to make this change, we shouldn't store the callback for CreatePlacementGroup. We should always reply right away like actor cases

Currently, the creation is replied "after" registration is done. I am not against this idea, but then please fix this issue.

clay4444 · 2021-02-03T12:03:59Z

discussed with @rkooo567 offline, we should return success immediately when HandleCreatePlacementGroup instead of returning the result after successful creation.

ffbin · 2021-02-17T11:41:30Z

src/ray/gcs/gcs_server/gcs_placement_group_manager.h

+  /// Callbacks of pending `RegisterPlacementGroup` requests.
+  /// Maps placement group ID to placement group registration callbacks, which is used to
+  /// filter duplicated messages from a driver/worker caused by some network problems.
+  absl::flat_hash_map<PlacementGroupID, std::vector<StatusCallback>>


Why change it to placement_group_to_register_callbacks_? I'm afraid the design is a bit redundant.

just like the registration of Actor, there will be the deconstruction problem if we just ignore the previous callback, I've checked this from @raulchen

@clay4444 Can you describe the specific problem in detail? thx

hm... I'm not sure whether I can explain this clearly, the general reason is that the GrpcClient will not be deconstructed since we just ignore the callback and no response was sent to the client

If it is a network exception, reply also cannot be returned.

src/ray/gcs/gcs_server/gcs_placement_group_manager.h

rkooo567 · 2021-02-21T03:28:16Z

Lint failure

rkooo567

A couple of last comments! After you address all of them, and if lint passes, I will merge the PR :)!

rkooo567 · 2021-02-21T03:28:46Z

src/ray/gcs/gcs_server/gcs_placement_group_manager.cc

-            iter->second(Status::NotFound(stream.str()));
-            placement_group_to_register_callback_.erase(iter);
-          }
+          // The placement group registration is synchronous, so if we found the placement


Great comment!

src/ray/gcs/gcs_server/gcs_placement_group_manager.cc

python/ray/tests/test_placement_group.py

src/ray/gcs/gcs_server/gcs_placement_group_manager.cc

clay4444 · 2021-02-22T05:26:37Z

src/ray/gcs/gcs_server/gcs_placement_group_manager.cc

          SchedulePendingPlacementGroups();
+        } else {
+          // The placement group registration is synchronous, so if we found the placement


After discussed with @ffbin , we don't need to invoke RAY_CHECK(placement_group_to_register_callback_.count(placement_group_id) == 0) here, because we will remove the placement group from registered_placement_groups_ and placement_group_to_register_callbacks_ only in the RemovePlacementGroup @rkooo567

Hmm can you at least make the log message to ERROR and say "This is a bug that should be addressed. Please report to Ray Github issue if you see this message"?

I still prefer to have a RAY_CHECK for strong consistency. What's the reason you guys prefer the log message? If we see the check failure, that means we just need to fix the issue rather than ignoring with the log messages.

rkooo567 · 2021-02-24T00:12:24Z

Wait I probably needed to have a API change approval.

rkooo567 · 2021-02-24T01:28:43Z

Okay approved :)

clay4444 added 8 commits December 21, 2020 17:10

Merge remote-tracking branch 'upstream/master'

f6dd551

Merge remote-tracking branch 'upstream/master'

6dadf24

Merge remote-tracking branch 'upstream/master'

f2f72b7

Merge remote-tracking branch 'upstream/master'

b6266a6

Merge remote-tracking branch 'upstream/master'

5589949

Merge remote-tracking branch 'upstream/master'

92347c0

Merge remote-tracking branch 'upstream/master'

b7352f4

make pg creation sync

de60068

clay4444 requested review from rkooo567 and ffbin February 2, 2021 15:18

clay4444 assigned rkooo567 and ffbin Feb 2, 2021

rkooo567 requested changes Feb 2, 2021

View reviewed changes

return successful immediately when pg registeration

c4bcb37

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

clay4444 added 10 commits February 5, 2021 15:33

hold on

fc686d7

fix ut

b67452a

make collection for callback

33a6414

Merge remote-tracking branch 'upstream/master'

ba0cac1

make pg registration vector

713a9fd

Merge remote-tracking branch 'upstream/master'

9248abf

Merge branch 'master' into make_create_pg_sync

44cd451

fix new cpp ut

a5fbf6d

fix named py ut

128ebff

fix python ut bug

33e2811

ffbin reviewed Feb 17, 2021

View reviewed changes

src/ray/gcs/gcs_server/gcs_placement_group_manager.h Outdated Show resolved Hide resolved

clay4444 added 2 commits February 18, 2021 10:40

modify comment

3d6950f

Merge remote-tracking branch 'upstream/master'

2e23325

clay4444 assigned ffbin, clay4444 and rkooo567 and unassigned ffbin and rkooo567 Feb 20, 2021

clay4444 added 3 commits February 20, 2021 14:27

Merge remote-tracking branch 'upstream/master'

35f9626

fix conflict

6537725

fix comment

9f6485c

rkooo567 approved these changes Feb 21, 2021

View reviewed changes

fix comment

473526e

ffbin reviewed Feb 22, 2021

View reviewed changes

src/ray/gcs/gcs_server/gcs_placement_group_manager.cc Outdated Show resolved Hide resolved

ffbin reviewed Feb 22, 2021

View reviewed changes

src/ray/gcs/gcs_server/gcs_placement_group_manager.cc Outdated Show resolved Hide resolved

clay4444 added 2 commits February 22, 2021 10:32

add new ut and fix old lint issue

9bdf8cc

fix comment

013fb37

clay4444 commented Feb 22, 2021

View reviewed changes

clay4444 added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Feb 22, 2021

clay4444 requested a review from rkooo567 February 22, 2021 08:02

clay4444 added 2 commits February 23, 2021 10:59

update comment

e2f7ea6

fix conflict

8b533cc

rkooo567 merged commit 911b028 into ray-project:master Feb 24, 2021

rkooo567 added the core-interface-change-approval-required This changes the Ray core behavior / API and requires broader approvals. label Feb 24, 2021

rkooo567 removed the core-interface-change-approval-required This changes the Ray core behavior / API and requires broader approvals. label Feb 24, 2021

clay4444 mentioned this pull request Sep 14, 2021

[Placement Group] Remove legacy code in placement group #18589

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Placement Group] Make the creation of placement group sync #13858

[Placement Group] Make the creation of placement group sync #13858

clay4444 commented Feb 2, 2021 •

edited

rkooo567 left a comment

clay4444 commented Feb 3, 2021

rkooo567 commented Feb 3, 2021

clay4444 commented Feb 3, 2021

rkooo567 commented Feb 3, 2021

clay4444 commented Feb 3, 2021

ffbin Feb 17, 2021 •

edited

clay4444 Feb 18, 2021

ffbin Feb 18, 2021

clay4444 Feb 18, 2021

ffbin Feb 22, 2021

rkooo567 commented Feb 21, 2021

rkooo567 left a comment

rkooo567 Feb 21, 2021

clay4444 Feb 22, 2021 •

edited

rkooo567 Feb 22, 2021

rkooo567 Feb 22, 2021

rkooo567 commented Feb 24, 2021

rkooo567 commented Feb 24, 2021

[Placement Group] Make the creation of placement group sync #13858

[Placement Group] Make the creation of placement group sync #13858

Conversation

clay4444 commented Feb 2, 2021 • edited

Why are these changes needed?

Related issue number

Checks

rkooo567 left a comment

Choose a reason for hiding this comment

clay4444 commented Feb 3, 2021

rkooo567 commented Feb 3, 2021

clay4444 commented Feb 3, 2021

rkooo567 commented Feb 3, 2021

clay4444 commented Feb 3, 2021

ffbin Feb 17, 2021 • edited

Choose a reason for hiding this comment

clay4444 Feb 18, 2021

Choose a reason for hiding this comment

ffbin Feb 18, 2021

Choose a reason for hiding this comment

clay4444 Feb 18, 2021

Choose a reason for hiding this comment

ffbin Feb 22, 2021

Choose a reason for hiding this comment

rkooo567 commented Feb 21, 2021

rkooo567 left a comment

Choose a reason for hiding this comment

rkooo567 Feb 21, 2021

Choose a reason for hiding this comment

clay4444 Feb 22, 2021 • edited

Choose a reason for hiding this comment

rkooo567 Feb 22, 2021

Choose a reason for hiding this comment

rkooo567 Feb 22, 2021

Choose a reason for hiding this comment

rkooo567 commented Feb 24, 2021

rkooo567 commented Feb 24, 2021

clay4444 commented Feb 2, 2021 •

edited

ffbin Feb 17, 2021 •

edited

clay4444 Feb 22, 2021 •

edited