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

Storage DriverInfo has redundant fields `MaxSize` and `SupportedSizeRange` #83689

Closed
davidz627 opened this issue Oct 9, 2019 · 5 comments

Comments

@davidz627
Copy link
Contributor

commented Oct 9, 2019

Since this refactor added SupportedSizeRange: #78306
We seem to have these duplicate fields where SupportedSizeRange is used in most places and MaxSize is only used in one specific spot.

MaxSize is currently only used here:

fileSizes := createFileSizes(dInfo.MaxFileSize)

Should we just remove that one usage of MaxSize? Not sure what the deprecation policy is on our exported struct and whether we can just remove that variable (or just mark as deprecated and not used).

@davidz627

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

/sig storage
/cc @jsafrane @hoyho @msau42

@k8s-ci-robot k8s-ci-robot added sig/storage and removed needs-sig labels Oct 9, 2019
@tedyu

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

 just mark as deprecated and not used

+1

@davidz627

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2019

Just saw this again and don't know what I was thinking. Seems like MaxFileSize is referring to file size for writing into the disk during tests - SupportedSizeRange is about the disk size itself. Was confused because SupportedSizeRange does not specify the size of what. These fields are not redundant.

@davidz627 davidz627 closed this Oct 18, 2019
@tedyu

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2019

@davidz627
How about adding comment above the two fields so that people looking at this code in the future would have better idea ?

I can send out a PR for this.

@davidz627

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2019

yeah I think that would be good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.