Skip to content

refactor: move from io/ioutil to io and os package#998

Merged
yvrhdn merged 1 commit intografana:mainfrom
Juneezee:deprecate-ioutil
Sep 30, 2021
Merged

refactor: move from io/ioutil to io and os package#998
yvrhdn merged 1 commit intografana:mainfrom
Juneezee:deprecate-ioutil

Conversation

@Juneezee
Copy link
Copy Markdown
Contributor

What this PR does:
The io/ioutil package has been deprecated in Go 1.16 (See https://golang.org/doc/go1.16#ioutil). Since tempo has upgraded to Go 1.17 (#953), this PR replaces the existing io/ioutil functions with their new definitions in io and os packages. This change is transparent to end users.

Which issue(s) this PR fixes:
N/A

Checklist

  • [ ] Tests updated
  • [ ] Documentation added
  • [ ] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Sep 30, 2021

CLA assistant check
All committers have signed the CLA.

@yvrhdn
Copy link
Copy Markdown
Contributor

yvrhdn commented Sep 30, 2021

Hi 👋🏻 Thank you for this! I'll let the CI run.

Did you do this all manually or is there some kind of tool out there?

@Juneezee
Copy link
Copy Markdown
Contributor Author

Hi 👋🏻 Thank you for this! I'll let the CI run.

Did you do this all manually or is there some kind of tool out there?

Hi, thank you! I used the Search function in VSCode

Copy link
Copy Markdown
Contributor

@yvrhdn yvrhdn left a comment

Choose a reason for hiding this comment

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

Thanks for this, this is a great first contribution! Besides my comment below it all looks good 👍🏻

Comment thread tempodb/wal/wal.go
}

start := time.Now()
level.Info(log).Log("msg", "beginning replay", "file", f.Name(), "size", f.Size())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's unfortunate we are losing this information here (but I get os.ReadDir is more performant this way). I'll check with the team to see whether we want to keep this information or not.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the os.DirEntry interface does not have the Size() method 😞. If we want to keep the size in the log, we would need to do something like this:

fileInfo, err := f.Info()
if err != nil {
    return nil, err
}
level.Info(log).Log("msg", "beginning replay", "file", f.Name(), "size", fileInfo.Size())

Comment thread tempodb/wal/wal.go Outdated

start := time.Now()
level.Info(log).Log("msg", "beginning replay", "file", f.Name(), "size", f.Size())
level.Info(log).Log("msg", "beginning replay", "file", f.Name())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can grab the file size using DirEntry.Info and FileInfo.Size. Can you add the size label again? Then this PR will not change anything 🙂

The io/ioutil package has been deprecated as of Go 1.16, see
https://golang.org/doc/go1.16#ioutil. This commit replaces the existing
io/ioutil functions with their new definitions in io and os packages.

Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
Comment thread tempodb/wal/wal.go
Comment on lines +92 to +97
fileInfo, err := f.Info()
if err != nil {
return nil, err
}

level.Info(log).Log("msg", "beginning replay", "file", f.Name(), "size", fileInfo.Size())
Copy link
Copy Markdown
Contributor Author

@Juneezee Juneezee Sep 30, 2021

Choose a reason for hiding this comment

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

@kvrhdn Done 😃, please take a look again. The err here is not nil when the file f is removed after os.ReadDir is called, it's a rare error but still possible sometimes.

@Juneezee Juneezee requested a review from yvrhdn September 30, 2021 14:20
Copy link
Copy Markdown
Contributor

@yvrhdn yvrhdn left a comment

Choose a reason for hiding this comment

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

Thanks again!

@yvrhdn yvrhdn merged commit 5ab55db into grafana:main Sep 30, 2021
@yvrhdn yvrhdn mentioned this pull request Sep 30, 2021
3 tasks
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.

3 participants