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.

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

Fix comparison for expect_connecting_localities config entry #3486

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
2 participants
@msimberg
Copy link
Contributor

commented Oct 9, 2018

I think this fixes #3453, although I'm not quite sure if there are side effects with multiple localities. This at least works for me with a single locality.

The expect_connections variable was set opposite to what the comment above it says, and the comment sounds like the correct thing to do, so I've changed it to expect connections only if the number of localities is greater than one (or if it's set explicitly of course).

@khuck could you check if this works for phylanx?

@hkaiser

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

The expect_connections variable was set opposite to what the comment above it says, and the comment sounds like the correct thing to do, so I've changed it to expect connections only if the number of localities is greater than one (or if it's set explicitly of course).

@msimberg why shouldn't we expect connections if the application runs on one locality only?

@msimberg

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2018

Please correct me if I'm wrong but I thought expect_connecting_localities is exactly for localities connecting dynamically after initialization. If I've set one locality I would think that's in most cases because I really only want to use one locality and don't expect any others to connect, but if that's not what I want I can still set expect_connecting_localities=1.

That said, this does seem to cause problems on some tests so it may not be the way to go.

In any case, either the comment or the code is wrong.

@hkaiser

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

@msimberg yes, makes sense.

msimberg added some commits Oct 9, 2018

Fix comparison for expect_connecting_localities config entry
The old version expected connecting localities if the number of localities
was 1 and not if larger than 1, opposite of what is should be.
Fix launch_process test
Explicitly say that we expect connecting localities

@msimberg msimberg force-pushed the msimberg-patch-1 branch from 723795b to ff0595d Oct 23, 2018

@msimberg msimberg changed the title Fix comparison for expect_connecting_localities config entry WIP: Fix comparison for expect_connecting_localities config entry Oct 27, 2018

@msimberg msimberg modified the milestones: 1.2.0, 1.3.0 Nov 1, 2018

@msimberg msimberg changed the title WIP: Fix comparison for expect_connecting_localities config entry Fix comparison for expect_connecting_localities config entry Nov 22, 2018

@msimberg

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

Note: this passes all tests now. I had to change the following:

  • always enable parcel ports when in batch environments
  • don't notify waiting main if errors happen before main is waiting for finalize

The fixes are not the prettiest, but seem to work.

@msimberg

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

Closing since #3599 was merged.

@msimberg msimberg closed this Jan 7, 2019

@@ -272,7 +272,7 @@ namespace hpx { namespace components { namespace server
{
///////////////////////////////////////////////////////////////////////////
runtime_support::runtime_support(hpx::util::runtime_configuration & cfg)
: stop_called_(false), stop_done_(false), terminated_(false),
: stop_called_(true), stop_done_(true), terminated_(false),
dijkstra_color_(false), shutdown_all_invoked_(false),

This comment has been minimized.

Copy link
@hkaiser

hkaiser Jan 7, 2019

Member

@msimberg: Is that change still necessary?

This comment has been minimized.

Copy link
@msimberg

msimberg Jan 7, 2019

Author Contributor

Good catch. In principle yes, since the change is independent from the networking changes. It just happened to be triggered once these changes were introduced. I'll check and open a new PR with that change only if needed.

@hkaiser hkaiser deleted the msimberg-patch-1 branch Jan 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.