[core] Fix error info timestamp unit#48763
Conversation
Signed-off-by: dentiny <dentinyhao@gmail.com>
|
Where do you find the original |
Lines 80 to 81 in 16a6e45
|
|
@rynewang Somehow I think the purpose for the fix is more of "fixate a unit of timestamp" rather than "get the original intention for what timestamp unit we use". |
Signed-off-by: dentiny <dentinyhao@gmail.com>
|
@rynewang Do you have any comments on this PR? Thanks! |
Signed-off-by: dentiny <dentinyhao@gmail.com>
|
some cpp tests are failing on this |
Signed-off-by: hjiang <hjiang@anyscale.com>
src/ray/gcs/pb_utils.cc
Outdated
| } else { | ||
| error_info_ptr->set_error_message(error_msg); | ||
| } | ||
| error_info_ptr->set_timestamp(absl::ToUnixSeconds(timestamp)); |
There was a problem hiding this comment.
this limits time resolution to seconds, which is not acceptable. let's use milliseconds, or ToDoubleSeconds. Either way, please annotate the proto on what unit it uses.
There was a problem hiding this comment.
this limits time resolution to seconds, which is not acceptable.
But I think from the code pointer I referenced, it's intended to be seconds at the very beginning.
Do you mean we should change the semantics, whatever it is now?
There was a problem hiding this comment.
or ToDoubleSeconds
I'm not sure what do you mean here.
It's made for absl::Duration, instead of absl::Time.
https://github.com/abseil/abseil-cpp/blob/2fcebef792241578e8c9dcfbba5745f834509dbb/absl/time/time.h#L647
There was a problem hiding this comment.
Updated to millisecond as you wish, also add a comment for unit in the protobuf.
But checking the code base, if we want to update the field name, there're way too many places to update in the PR, I leave a TODO.
There was a problem hiding this comment.
This PR has already been over a week, I strongly prefer to merge it if no other issues.
Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
Addresses ray-project#48760 Reading through the initial PR (https://github.com/ray-project/ray/pull/2256/files), the timestamp unit for error information is actually seconds. After refactor PR (ray-project#5024), it's refactored from python class into protobuf. In this PR, I tried to use `absl::Time` instead of integer as argument, to avoid confusion on units. --------- Signed-off-by: dentiny <dentinyhao@gmail.com> Signed-off-by: hjiang <hjiang@anyscale.com> Co-authored-by: Ruiyang Wang <56065503+rynewang@users.noreply.github.com> Signed-off-by: Connor Sanders <connor@elastiflow.com>
Addresses #48760
Reading through the initial PR (https://github.com/ray-project/ray/pull/2256/files), the timestamp unit for error information is actually seconds.
After refactor PR (#5024), it's refactored from python class into protobuf.
In this PR, I tried to use
absl::Timeinstead of integer as argument, to avoid confusion on units.