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

Refactor bootstrap checks #16844

Closed
wants to merge 4 commits into from
Closed

Refactor bootstrap checks #16844

wants to merge 4 commits into from

Conversation

jasontedor
Copy link
Member

This commit refactors the bootstrap checks into a dedicated class. The
refactoring provides a model for different limits per operating system,
and provides a model for unit tests for individual checks.

Relates #16733, relates #16835

@jasontedor jasontedor added resiliency and removed :Core/Infra/Core Core issues without another label labels Feb 28, 2016
@rmuir
Copy link
Contributor

rmuir commented Feb 28, 2016

I guess for me what is broken, is the implication that if i want to bind to some address other than the default, that this means i'm gonna overload my machine with thousands of shards.

I don't change things like file descriptors limits on my computer because i really don't want to have non-default values here as a developer. Now what to do... I suppose this effectively stops me from working on the networking code at the very least.

This commit refactors the bootstrap checks into a dedicated class. The
refactoring enables different limits for snapshot versus release builds
and provides a model for unit tests for individual checks.
@s1monw
Copy link
Contributor

s1monw commented Feb 29, 2016

@jasontedor I guess we should go with an explicit setting for now? I think it's less controversial? the other option is to move to an opt out setting?

@jaymode
Copy link
Member

jaymode commented Feb 29, 2016

the other option is to move to an opt out setting?

+1. If the error message tells you that you can disable this check with setting foo but never do it in production, I think that would be a good compromise?

@nik9000
Copy link
Member

nik9000 commented Feb 29, 2016

+1. If the error message tells you that you can disable this check with setting foo but never do it in production, I think that would be a good compromise?

I prefer opt out as well. You have to intentionally think "I don't care about safe stuff, just do things". And gradle run can just set it all the time.

@jaymode
Copy link
Member

jaymode commented Feb 29, 2016

I'll caveat my earlier comment, if we are adding a setting then I prefer opt-out. I think its a fine line between preventing users from shooting themselves in the foot and making it harder to do development when a change is made like setting network.host so two developers could collaborate on a change together.

@s1monw
Copy link
Contributor

s1monw commented Mar 1, 2016

to be honest I don't like either way. The fact that OSX is a pain to get this configured correctly is concerning and given the fact that OSX is unlikely a production env we can maybe opt out automatically if we run on OSX? I mean all these checks are best effort so I guess that's not that much of a problem. what do you think @jasontedor just use this constant: Constants.MAC_OS_X

@jasontedor
Copy link
Member Author

The fact that OSX is a pain to get this configured correctly is concerning and given the fact that OSX is unlikely a production env we can maybe opt out automatically if we run on OS X?

I am not in favor of opting out automatically on OS X, but I am in favor of reduced limits for the file descriptor check on OS X. I think that future checks can be handled on a case-by-case basis.

@jasontedor
Copy link
Member Author

@rmuir @s1monw I pushed 783fbd2 which drops the file descriptor check on OS X to the default setting for that OS, and removed the different checks between snapshot and release builds.

@s1monw
Copy link
Contributor

s1monw commented Mar 2, 2016

LGTM

This commit adds a test of the situation when the current file
descriptor count is not available.
@jasontedor jasontedor closed this in 9ad5919 Mar 2, 2016
@jasontedor jasontedor deleted the bootstrap-checks branch March 2, 2016 13:48
@jasontedor jasontedor removed the review label Mar 2, 2016
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

6 participants