-
Notifications
You must be signed in to change notification settings - Fork 59
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
create unique task_id with timestamp #223
Conversation
Signed-off-by: youliang <tan_you_liang@hotmail.com>
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. Can you update the CHANGELOG?
if (use_timestamp_for_task_id) | ||
{ | ||
task_id += std::to_string( | ||
std::chrono::duration_cast<std::chrono::seconds>( |
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, why do we need the duration_cast? time_since_epoch().count()
would directly return the nanoseconds since the start.
Also, should we use node time here? (node->get_clock()->now()
)
Signed-off-by: youliang <tan_you_liang@hotmail.com>
if (use_timestamp_for_task_id) | ||
{ | ||
task_id += std::to_string( | ||
static_cast<int>(node->get_clock()->now().nanoseconds()/1e9)); |
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.
Wondering if we should leave the timestamp in nanoseconds and not divide by 1e9 to get seconds? If somehow two or more tasks get processed within the same second, they might have the same task_id?
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.
Yea, that is valid concern that i thought about too. The only concern (probably insignificant) is having nanosec as timestamp is way too long. This will result in a long task_id
that might look quite ugly on the dashboard? Example shown here:
Let me nego abit, how about milisec? Since unix_millis
time is regularly used when defining our rmf time field.
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.
Okay let's go with millis for now.
I do this this feature will be redundant once we implement a proper backup and restore functionality for this node. That way when the node is restored after a crash, it will know the last issued task_id.
Signed-off-by: youliang <tan_you_liang@hotmail.com>
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.
Thanks to the approval and addressing the failed CI #225 . Will merge this in since the failed CI is not relevant to this pr |
* create unique task_id with timestamp Signed-off-by: youliang <tan_you_liang@hotmail.com> * use node time and update changelog Signed-off-by: youliang <tan_you_liang@hotmail.com> * change to milli unix sec Signed-off-by: youliang <tan_you_liang@hotmail.com> Signed-off-by: youliang <tan_you_liang@hotmail.com> Signed-off-by: Yadunund <yadunund@gmail.com>
The current way of generating a task id uses a
task_counter
, which generates the task_id by appending an integertask counter
value (eg.patrol.dispatch-1
). However, this poses a problem when we require the task_id to be unique even after restarting thetask dispatcher
node (during deployment setup especially). Therefore, this solution generates a unique id with a timestamp. This can be enabled via ros2 param, by settinguse_timestamp_for_task_id
astrue
.p/s:
use_timestamp_for_task_id
is false by default (using task counter)To test
Run dispatcher with use_timestamp_for_task_id as true
Try dispatch a dummy task