-
Notifications
You must be signed in to change notification settings - Fork 3
[flowstate_ros_bridge] Make page_size and other params configurable in ExecutiveBridge #72
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
Conversation
mjcarroll
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with small nits
flowstate_ros_bridge/include/flowstate_ros_bridge/executive.hpp
Outdated
Show resolved
Hide resolved
| const std::string &solution_service_address, | ||
| std::size_t deadline_seconds = 5, | ||
| std::size_t update_rate_millis = 1000); | ||
| std::size_t update_rate_millis = 1000, std::size_t page_size = 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed this with @luca-della-vedova who suggested relying on next_page_token to recursively get all pages. We change the arg to std::optional<std::size_t> with default nullopt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually upon further thought I feel we should remove the parameter altogether and instead:
- In the
clear_and_delete_operationshere, always iterate through all operations. The function's API promises to delete all currently running operations, deleting only one page goes against that. - In the
behavior_treeshere, either do the same as the above or make it a function parameter with default value, i.e.:
// Current
absl::StatusOr<std::vector<BehaviorTree>> behavior_trees() const;
// After, return the first `n` or all the behavior trees.
absl::StatusOr<std::vector<BehaviorTree>> behavior_trees(std::optional<std::size_t> n = std::nullopt) const;
But still my recommendation would still be to remove it altogether since it's not clear what the ordering of the behavior trees returned would be, and "first n" might mean anything from "alphabetical order", "last edit order", "creation order", non-guaranteed order, and unless we can guarantee a fixed ordering users should probably not rely on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
The flowstate_ros_bridge queries the Flowstate Solution Servce to get a list of BTs. See here. The default request params will only query 20 BTs as seen here. Another area where
page_sizeis used is querying the execute for a list of running operations using this proto.If your solution had more than 20 BTs, calling the service to list the behavior trees would return a truncated list.
This PR
page_sizeconfigurable and by default sets it to the max value.