Skip to content

GODRIVER-2674 use time.Duration instead of durationNanos #1120

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

GODRIVER-2674 use time.Duration instead of durationNanos #1120

wants to merge 2 commits into from

Conversation

jtagcat
Copy link
Contributor

@jtagcat jtagcat commented Nov 6, 2022

Same as #1105

OK make fmt
OK make lint
make test: go.mongodb.org/mongo-driver/mongo/integration/unified: no such command: 'configureFailPoint'
(make test fails on master with same result)
make test-race: same result as make test

@matthewdale matthewdale self-requested a review November 9, 2022 20:41
@kevinAlbs kevinAlbs requested review from qingyang-hu and removed request for matthewdale December 5, 2022 20:43
qingyang-hu
qingyang-hu reviewed Dec 13, 2022
View reviewed changes
Copy link
Collaborator

@qingyang-hu qingyang-hu left a comment

Choose a reason for hiding this comment

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

The replacement seems reasonable though it is backward-breaking.
Instead, in v1.x, we can deprecate the current fields while adding new fields named Duration that are type time.Duration.
We can retire these DurationNanos fields in the upcoming v2 driver.

qingyang-hu
qingyang-hu reviewed Dec 13, 2022
View reviewed changes
@matthewdale matthewdale changed the title use time.Duration instead of durationNanos GODRIVER-2674 use time.Duration instead of durationNanos Dec 13, 2022
jtagcat
jtagcat commented Dec 14, 2022
View reviewed changes
mongo/integration/unified/client_entity.go
AppendString("name", "CommandFailedEvent").
AppendDouble("observedAt", getSecondsSinceEpoch()).
AppendInt64("durationNanos", evt.DurationNanos).
AppendInt64("duration", int64(evt.Duration)).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is the bson used? Why do other events not use Duration (non-exhaustive)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This BSON document is used for test assertions. It must remain the same for the test assertions to continue working.

matthewdale
matthewdale requested changes Feb 15, 2023
View reviewed changes
Copy link
Collaborator

@matthewdale matthewdale left a comment

Choose a reason for hiding this comment

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

Hey @jtagcat sorry about the slow review and thanks for the edits! I have one required change, but it otherwise looks great!

mongo/integration/unified/client_entity.go
AppendString("name", "CommandFailedEvent").
AppendDouble("observedAt", getSecondsSinceEpoch()).
AppendInt64("durationNanos", evt.DurationNanos).
AppendInt64("duration", int64(evt.Duration)).
Copy link
Collaborator

Choose a reason for hiding this comment

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

This BSON document is used for test assertions. It must remain the same for the test assertions to continue working.

mongo/integration/unified/client_entity.go
AppendString("name", "CommandFailedEvent").
AppendDouble("observedAt", getSecondsSinceEpoch()).
AppendInt64("durationNanos", evt.DurationNanos).
AppendInt64("duration", int64(evt.Duration)).
Copy link
Collaborator

Choose a reason for hiding this comment

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

The structure of this BSON document must remain the same for test assertions to continue working.

Suggested change
AppendInt64("duration", int64(evt.Duration)).
AppendInt64("durationNanos", duration.Nanoseconds()).

@matthewdale matthewdale mentioned this pull request Feb 27, 2023
Merged
@matthewdale
Copy link
Collaborator

Closing in favor of #1193

@matthewdale matthewdale closed this Mar 1, 2023
@jtagcat jtagcat deleted the durationTime branch March 1, 2023 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qingyang-hu qingyang-hu qingyang-hu left review comments

@matthewdale matthewdale matthewdale requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jtagcat @matthewdale @qingyang-hu