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

gapit: Add OutputCSV flag to benchmark (#2540). #2573

Merged
merged 3 commits into from Feb 1, 2019

Conversation

pau-baiget
Copy link
Collaborator

This flag produces a single row of comma-separated data.
Also, all time information is expressed in milliseconds.

This flag produces a single row of comma-separated data.
Also, all time information is expressed in milliseconds.
w.Flush()
if verb.OutputCSV {
csvWriter := csv.NewWriter(os.Stdout)
defer csvWriter.Flush()
Copy link
Contributor

Choose a reason for hiding this comment

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

Defer is a bit funny (and you probably don't actually want it here) but defer actually defers until the end of the current function (NOT the scope-block).
How I typically handle this is:

if verb.OutputCSV { 
   func() {
      csvWriter....
      defer csvwriter.Flush()
      ....
      ....
   } () 
 } else {
      ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah! Ok, didn't know that. Will change it and resubmit.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is super subtle, and goes completely against all of my past experience (thanks Go)

Copy link
Contributor

@AWoloszyn AWoloszyn left a comment

Choose a reason for hiding this comment

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

One small nit about defer, and the presubmit is failing, other than that, looks good to me.

Copy link
Contributor

@AWoloszyn AWoloszyn left a comment

Choose a reason for hiding this comment

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

Please make sure to squash before commit.

@pau-baiget pau-baiget merged commit a64a587 into google:master Feb 1, 2019
@pau-baiget pau-baiget deleted the add-csv-benchmark branch February 1, 2019 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants