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

Set Go Max procs in a better location #2147

Merged
merged 4 commits into from
Apr 2, 2015

Conversation

corylanou
Copy link
Contributor

Go max procs doesn't always need to be set. It doesn't really hurt anything, but the log statement had an undesired side affect.

  1. It was printing above the logo, so it felt out of place when the server started up.
  2. For thinks like printing usage, or config, it was being written out and again, felt out of place.

@@ -78,11 +74,13 @@ func main() {
case "":
execRun(args)
case "backup":
setGoMaxProcs()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do backup and restore really benefit from this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question for @benbjohnson. Not sure if more procs helps out with any of the system packages we depend on.

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: why do it in the switch statement for some cases, but then do it explicitly in execRun for that case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's necessary. Backup & restore is single threaded since it builds a tar archive sequentially.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@otoolep looks like we can remove it from everywhere but run. However, the reason I didn't put it in the switch statement for the run is it printed above the logo, and felt out of place. I'll just leave that in the run and remove all of them from the switch, as well as the actual function now that we are clear on the use case.

@otoolep
Copy link
Contributor

otoolep commented Apr 2, 2015

I think I would just push the call into execRun and leave it at that. I don't have a strong opinion on this, however. In other words move it out of the very top-level, move the two lines to execRun and don't create a new function.

corylanou added a commit that referenced this pull request Apr 2, 2015
@corylanou corylanou merged commit 734e57a into master Apr 2, 2015
@corylanou corylanou deleted the set-max-procs-better-location branch April 2, 2015 15:42
mark-rushakoff pushed a commit that referenced this pull request Jan 11, 2019
fix(http): rename piging_test to paging_test
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.

4 participants