-
Notifications
You must be signed in to change notification settings - Fork 23
[OCTRL-991]: Include timestamp in state related gRPC calls #665
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
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.
Thank you! My only concern is related to the point in time that these timestamps are taken. Sometimes it is at the moment of receiving the request, sometimes just before replying. If the request processing takes very little time, I guess it does not matter, but otherwise it could potentially be confusing. How about taking the timestamp always at the beginning of the call? This way it should bring minimal boilerplate code, but keep the behaviour consistent.
|
I changed the code to address what you said. Please don't merge this after approving, I need to squash the fixup commit after. |
|
uhm... I just now noticed, that I cannot read and put timestamp at the end of the call not beginning. Sorry Should I redo it, or is this fine? |
At the end is also fine with me, as long as the behaviour is consistent. But since George is back, perhaps you can check with him. |
|
it can be merged now. I applied all fixup comments, so history is clean a george cofirmed that everything is working for him |
I added timestamp to all gRPC calls replies which is int64 unix timestamp in milliseconds.