Skip to content
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

Record the stream and consumer info timestamps #4133

Merged
merged 1 commit into from Jun 2, 2023

Conversation

ripienaar
Copy link
Contributor

This records the server time when info for streams and consumers are created so that tools such as the nats cli can calculate time deltas for last ack, last delivered and so forth in the context of the server clock.

This will help aleviate problems with client devices experiencing clock jitter that can show up in user interfaces as negative seconds since last ack etc

@ripienaar ripienaar requested a review from a team as a code owner May 5, 2023 14:41
server/consumer.go Outdated Show resolved Hide resolved
This records the server time when info for streams and
consumers are created so that tools such as the nats cli
can calculate time deltas for last ack, last delivered and
so forth in the context of the server clock.

This will help aleviate problems with client devices experiencing
clock jitter that can show up in user interfaces as negative
seconds since last ack etc

Signed-off-by: R.I.Pienaar <rip@devco.net>
@ripienaar ripienaar force-pushed the consumer_stream_info_times branch from 8dd7474 to fb1d86d Compare May 5, 2023 16:53
Copy link
Member

@wallyqs wallyqs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@derekcollison derekcollison self-requested a review May 5, 2023 18:06
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Should we consider that when this is used elsewhere it is called Now() in the struct?

@wallyqs
Copy link
Member

wallyqs commented May 5, 2023

JSInfo uses Now too, maybe standardize on using that for the struct name instead?

Now time.Time `json:"now"`

@ripienaar
Copy link
Contributor Author

To me, now just seems wrong :) I think since we already use ts in the jetstream API as wally points out we should stick to that

@ripienaar
Copy link
Contributor Author

All monitor things using now is good, all api things using the same is alao good - better if all used the same of course, but we cant change the js api ones so better be consistent in the api?

@derekcollison
Copy link
Member

I am not hard set on either one, just brought it up to gather thoughts and opinions.

I think since we are putting it in Stream and Consumer info should match the precedent set in JetStream to date, which sounds like what we already have in this PR.

@ripienaar ripienaar merged commit c24547e into nats-io:dev Jun 2, 2023
1 check passed
@ripienaar ripienaar deleted the consumer_stream_info_times branch June 2, 2023 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants