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

Add `-w` and `-t` options to roscore. #687

Closed
wants to merge 1 commit into
base: indigo-devel
from

Conversation

Projects
None yet
4 participants
@lucasb-eyer
Contributor

lucasb-eyer commented Oct 15, 2015

These define the number of worker-threads of the master and the socket timeout to use.

For large projects, the default of 3 workers with the OS' default timeout (of roughly 2-3mins) will cause the topic notification queue to stall as soon as a computer doesn't answer. So large projects want to set the timeout much lower and increase the thread-pool size a bit.

I'd like to add that having to define the option in three places and pass it through multiple objects almost made me go insane.

Ping @tlind as he debugged this together with me.

@ros-pull-request-builder

This comment has been minimized.

Show comment
Hide comment
@ros-pull-request-builder

ros-pull-request-builder Oct 15, 2015

Member

Can one of the admins verify this patch?

Member

ros-pull-request-builder commented Oct 15, 2015

Can one of the admins verify this patch?

@dirk-thomas

View changes

Show outdated Hide outdated tools/rosmaster/src/rosmaster/master_api.py Outdated
@dirk-thomas

View changes

Show outdated Hide outdated tools/roslaunch/scripts/roscore Outdated
@dirk-thomas

View changes

Show outdated Hide outdated tools/roslaunch/scripts/roscore Outdated
@dirk-thomas

View changes

Show outdated Hide outdated tools/roslaunch/scripts/roscore Outdated
@dirk-thomas

View changes

Show outdated Hide outdated tools/roslaunch/src/roslaunch/nodeprocess.py Outdated
@dirk-thomas

View changes

Show outdated Hide outdated tools/rosmaster/src/rosmaster/master_api.py Outdated
@lucasb-eyer

This comment has been minimized.

Show comment
Hide comment
@lucasb-eyer

lucasb-eyer Oct 15, 2015

Contributor

I believe I have addressed all of your concerns. Note, though, that with this change NUM_WORKERS might not reflect the truth, as it's only used as default value. I can instead make it reflect the true value if you want.

Contributor

lucasb-eyer commented Oct 15, 2015

I believe I have addressed all of your concerns. Note, though, that with this change NUM_WORKERS might not reflect the truth, as it's only used as default value. I can instead make it reflect the true value if you want.

@dirk-thomas

View changes

Show outdated Hide outdated tools/roslaunch/scripts/roscore Outdated
@dirk-thomas

View changes

Show outdated Hide outdated tools/roslaunch/scripts/roscore Outdated
@dirk-thomas

View changes

Show outdated Hide outdated tools/roslaunch/src/roslaunch/__init__.py Outdated
@dirk-thomas

View changes

Show outdated Hide outdated tools/roslaunch/src/roslaunch/launch.py Outdated
@ros-pull-request-builder

This comment has been minimized.

Show comment
Hide comment
@ros-pull-request-builder
Member

ros-pull-request-builder commented Oct 25, 2015

Test passed.
Refer to this link for build results: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/423/

Add `-w` and `-t` options to roscore.
These define the number of worker-threads of the master and the socket
timeout to use.

For large projects, the default of 3 workers with the OS' default
timeout (of roughly 2-3mins) will cause the topic notification queue to
stall as soon as a computer doesn't answer. So large projects want to
set the timeout much lower and increase the thread-pool size a bit.
@lucasb-eyer

This comment has been minimized.

Show comment
Hide comment
@lucasb-eyer

lucasb-eyer Oct 25, 2015

Contributor

I addressed all comments, except for the Float type one, see my inline comment for why.

Contributor

lucasb-eyer commented Oct 25, 2015

I addressed all comments, except for the Float type one, see my inline comment for why.

@lucasb-eyer

This comment has been minimized.

Show comment
Hide comment
@lucasb-eyer

lucasb-eyer Oct 25, 2015

Contributor

The failure seems to be git-related: FATAL: Could not checkout null with start point 8c7de6996601395f6cb28a424f548d3f232f6503, not sure why though.

Contributor

lucasb-eyer commented Oct 25, 2015

The failure seems to be git-related: FATAL: Could not checkout null with start point 8c7de6996601395f6cb28a424f548d3f232f6503, not sure why though.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Nov 3, 2015

Member

Since roslaunch uses a new command line option of rosmaster it must add a minimum version to the dependency. Since it is not yet clear when this will be merged I will add it during the merge process.

Member

dirk-thomas commented Nov 3, 2015

Since roslaunch uses a new command line option of rosmaster it must add a minimum version to the dependency. Since it is not yet clear when this will be merged I will add it during the merge process.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas
Member

dirk-thomas commented Nov 3, 2015

@ros-pull-request-builder

This comment has been minimized.

Show comment
Hide comment
@ros-pull-request-builder
Member

ros-pull-request-builder commented Nov 3, 2015

Test passed.
Refer to this link for build results: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/428/

@lucasb-eyer

This comment has been minimized.

Show comment
Hide comment
@lucasb-eyer

lucasb-eyer Nov 3, 2015

Contributor

If I understand correctly, there's nothing more to do from my side, right?

Contributor

lucasb-eyer commented Nov 3, 2015

If I understand correctly, there's nothing more to do from my side, right?

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Nov 3, 2015

Member

Correct.

Member

dirk-thomas commented Nov 3, 2015

Correct.

@wjwwood

This comment has been minimized.

Show comment
Hide comment
@wjwwood

wjwwood Nov 7, 2015

Member

lgtm, but you should update the wiki in these places (at least) mentioning the new options:

Make sure to note what versions of ROS will contain this feature.

Member

wjwwood commented Nov 7, 2015

lgtm, but you should update the wiki in these places (at least) mentioning the new options:

Make sure to note what versions of ROS will contain this feature.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Nov 9, 2015

Member

Thanks, cherry-picked (including a version dependency) in: 08f6bf0

Member

dirk-thomas commented Nov 9, 2015

Thanks, cherry-picked (including a version dependency) in: 08f6bf0

@dirk-thomas dirk-thomas closed this Nov 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment