Clean up LegacyPhaseShim subscription after task_summaries removal#511
Clean up LegacyPhaseShim subscription after task_summaries removal#511Matthew0803 wants to merge 6 commits into
Conversation
Signed-off-by: Matthew0803 <matthewfc83@gmail.com>
Matthew0803
force-pushed
the
rm_legacy_reporting_mechanism
branch
from
8e573f1 to
49bde84
Compare
mxgrey
left a comment
mxgrey
left a comment
There was a problem hiding this comment.
I can understand having a default switch to Underway status at the start and a switch to Completed status at the end if the task accidentally finished with an Underway status, but I don't understand why would discard other status changes that happen in between. I've put more detailed comments inline.
| self->_last_state_value = msg.state; | ||
| if (msg.state == TaskSummary::STATE_QUEUED | ||
| || msg.state == TaskSummary::STATE_PENDING) | ||
| self->_state->update_status(Event::Status::Standby); | ||
| else if (msg.state == TaskSummary::STATE_ACTIVE) | ||
| self->_state->update_status(Event::Status::Underway); | ||
| else if (msg.state == TaskSummary::STATE_COMPLETED) | ||
| self->_state->update_status(Event::Status::Completed); | ||
| else if (msg.state == TaskSummary::STATE_FAILED) | ||
| self->_state->update_status(Event::Status::Failed); | ||
| else if (msg.state == TaskSummary::STATE_CANCELED) | ||
| self->_state->update_status(Event::Status::Canceled); | ||
| else | ||
| self->_state->update_status(Event::Status::Uninitialized); |
There was a problem hiding this comment.
I don't understand why we would remove the ability to update the task status based on the specific legacy TaskSummary::state value. With the changes in this PR, users would only ever get to see Underway or Failed status updates. What's the point of reducing the visible status updates to just that?
| } | ||
|
|
||
| /* *INDENT-OFF* */ | ||
| if (!self->_last_state_value.has_value() |
There was a problem hiding this comment.
What's the point of not tracking the last state value? Each time we change the task status we will be triggering communication with the server. We shouldn't do that if we're not delivering any new information.
Signed-off-by: Michael X. Grey <greyxmike@gmail.com>
Signed-off-by: Michael X. Grey <greyxmike@gmail.com>
mxgrey
left a comment
mxgrey
left a comment
There was a problem hiding this comment.
I've filtered this PR down to the key contributions and reverted what I felt was an unnecessary change in the way task statuses were being converted. I think this is good to merge now.
2 participants
Bug fix
#477
Fixed bug
The robot fleet system used to publish a "task status bulletin board" (/task_summaries topic) so other parts of the system could watch what robots were doing. That bulletin board was removed because nobody was reading it. But leftover code in LegacyPhaseShim still used the old bulletin board message format to figure out when a robot task started, finished, or failed, instead of just knowing those things directly.
Fix applied
Simplify the subscription code to: