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

PID dir can be specified for worker processes #174

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

briandamaged
Copy link

Added command-line argument that allows the PID folder to be specified for the worker processes.

(Why? Even though a user can launch multiple god processes, all of them write pids to the same ~/.god/pids directory. Thus, each god process can clobber the PID files for the other god processes. Specifying a separate --managed_pid_dir for each god process ensures that they will not clobber one another)

@@ -60,6 +61,10 @@ begin
options[:pid] = x
end

opts.on("-MDIR", "--managed_pid_dir DIR", "Where to write the PIDs for the workers") do |x|
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this setting would be fine without a short option. Could you remove the -M option?

Also, the command line options use - between words instead of _.

@briandamaged
Copy link
Author

Sounds good. I'm on it.

@briandamaged
Copy link
Author

Any ETA on merging this into master?

@briandamaged
Copy link
Author

Is there an ETA on merging this into master? My company has been running this branch since May 2014, so we're fairly certain that it works as expected.

@eric
Copy link
Collaborator

eric commented Jan 30, 2015

It looks like there's a separate change in here that is sleeping. Can you remove that?

@briandamaged
Copy link
Author

We found that the god command fails silently if it encounters an error in its configuration file. The sleep statement is a bit of a hack, but it allows the god command to verify that the workers are actually running before it declares victory.

Do you want me to move the sleep hack to a separate merge request? Or, would you rather that I remove the sleep hack entirely (and, possibly replace it w/ something more resilient)?

@eric
Copy link
Collaborator

eric commented Jan 30, 2015

I definitely think it should be a separate discussion. I'd be interested in talking about the sorts of errors you're talking about, but I'd rather if it was not mixed in with this.

@briandamaged
Copy link
Author

Alright, I removed the daemon launch verification logic. Sometime tomorrow, we can talk about the problem that it was intended to solve, and probably come up w/ a better way to address it.

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