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.

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

rbd: Remove redundant LastSeen property from MirrorPeerSite struct #871

Merged
merged 1 commit into from
Apr 28, 2023
Merged

rbd: Remove redundant LastSeen property from MirrorPeerSite struct #871

merged 1 commit into from
Apr 28, 2023

Conversation

Nikhil-Ladha
Copy link
Contributor

@Nikhil-Ladha Nikhil-Ladha commented Apr 18, 2023 •
edited
Loading

Description:
Removed the LastSeen property of MirrorPeerSite struct as it is not being set/updated anywhere by the ceph API.

Fixes: #870

Signed-off-by: Nikhil-Ladha nikhilladha1999@gmail.com

@phlogistonjohn phlogistonjohn added the API This PR includes a change to the public API of a go-ceph package label Apr 18, 2023
anoopcs9
anoopcs9 previously approved these changes Apr 21, 2023
View reviewed changes
Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

lgtm.

@phlogistonjohn
Copy link
Collaborator

I spoke to @ansiwen about this PR yesterday, I will give a few days for him to respond.

@mergify mergify bot dismissed anoopcs9’s stale review April 25, 2023 05:25

Pull request has been modified.

@Nikhil-Ladha Nikhil-Ladha changed the title rbd: Use int type instead of time_t in public APIs rbd: Use time.Time type instead of time_t in public APIs Apr 25, 2023
phlogistonjohn
phlogistonjohn reviewed Apr 27, 2023
View reviewed changes
Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

Maybe I'm being dense this morning, but don't we need to call a function to convert from a time_t to a time.Time? I'm kinda surprised that this doesn't fail the tests, maybe someone can explain to me what magic is happening.

@Nikhil-Ladha
Copy link
Contributor Author

Maybe I'm being dense this morning, but don't we need to call a function to convert from a time_t to a time.Time? I'm kinda surprised that this doesn't fail the tests, maybe someone can explain to me what magic is happening.

Ok, I see the issue.
We are not actually using the LastSeen property anywhere in the code, that's the reason there is no change for type casting. IIRC, the ceph code doesn't actually update the LastSeen property in the function, which begs the question should we care to add the property to the struct type here? Or, for the sake of semanticity we should lt it be?

@phlogistonjohn
Copy link
Collaborator

Maybe I'm being dense this morning, but don't we need to call a function to convert from a time_t to a time.Time? I'm kinda surprised that this doesn't fail the tests, maybe someone can explain to me what magic is happening.

Ok, I see the issue. We are not actually using the LastSeen property anywhere in the code, that's the reason there is no change for type casting. IIRC, the ceph code doesn't actually update the LastSeen property in the function, which begs the question should we care to add the property to the struct type here? Or, for the sake of semanticity we should lt it be?

We have some not-mutually-exclusive options then:

  • We could drop LastSeen from our struct. If it never gets set it is at best an attractive nuisance
  • We can keep it as a time.Time, with a conversion function, hoping that it is fixed in ceph some day
  • We can document the issue
  • If we keep the field we should write tests to ensure it behaves correctly (however we define correctly)
  • We can report this to the ceph issue tracker

Personally, I lean towards: dropping LastSeen from the struct, adding a line or two to the struct Doc comment explaining why there's no LastSeen, report the issue to ceph. I think it should be "safe" to drop the field for now because generally adding struct fields in the future is backwards compatible, so if LastSeen begins working someday we can add it back without much fuss while not bearing the costs of maintaing a non-functional field today.

Any other opinions: @anoopcs9 @ansiwen ?

@ansiwen
Copy link
Collaborator

ansiwen commented Apr 27, 2023

I fully agree, let's drop it, if we don't set it.

@anoopcs9
Copy link
Collaborator

Maybe I'm being dense this morning, but don't we need to call a function to convert from a time_t to a time.Time? I'm kinda surprised that this doesn't fail the tests, maybe someone can explain to me what magic is happening.

I completely missed to notice the effect of this change behind.

Personally, I lean towards: dropping LastSeen from the struct, adding a line or two to the struct Doc comment explaining why there's no LastSeen, report the issue to ceph. I think it should be "safe" to drop the field for now because generally adding struct fields in the future is backwards compatible, so if LastSeen begins working someday we can add it back without much fuss while not bearing the costs of maintaing a non-functional field today.

I am also in agreement here.

@Nikhil-Ladha
rbd: Remove redundant LastSeen property from MirrorPeerSite struct
9f9c9c1 
Removed the `LastSeen` property of MirrorPeerSite struct as it is not being set/updated anywhere by the ceph API.

Fixes: ceph#870

Signed-off-by: Nikhil-Ladha <nikhilladha1999@gmail.com>
@Nikhil-Ladha Nikhil-Ladha changed the title rbd: Use time.Time type instead of time_t in public APIs rbd: Remove redundant LastSeen property from MirrorPeerSite struct Apr 28, 2023
@Nikhil-Ladha
Copy link
Contributor Author

Made the suggested changes.

  • Removed the property from the struct
  • Opened a ceph issue to do the needful in the ceph code
  • Updated the PR, commit title and description

anoopcs9
anoopcs9 approved these changes Apr 28, 2023
View reviewed changes
Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks.

ansiwen
ansiwen approved these changes Apr 28, 2023
View reviewed changes
Copy link
Collaborator

@ansiwen ansiwen left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot merged commit 7476da2 into ceph:master Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anoopcs9 anoopcs9 anoopcs9 approved these changes

@ansiwen ansiwen ansiwen approved these changes

@phlogistonjohn phlogistonjohn Awaiting requested review from phlogistonjohn

Assignees
No one assigned
Labels
API This PR includes a change to the public API of a go-ceph package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rbd: mirror_peer_site.go exposes C type time_t to public api
4 participants
@Nikhil-Ladha @phlogistonjohn @ansiwen @anoopcs9