All of this code is required for custom args.
Here is no other things or implementations.
Can you comment concrete lines of code which are seem as excess?

joshuaspence added inline comments.Apr 27 2014, 10:33 AM

src/applications/daemon/management/PhabricatorDaemonManagementStatusWorkflow.php
6	This line, for example, seems to be unrelated.

rugabarbo added inline comments.Apr 27 2014, 11:04 AM

src/applications/daemon/management/PhabricatorDaemonManagementStatusWorkflow.php
6	We used this string one time before custom args implementation (see old implementation). And we use this string two times now (see new implementation). You can find twice usage of self::DEAD_NAME_PREFIX const. It's reasonable for me that I turned inline usage of string '<DEAD> ' into const. Are you suggest to write bad code and then refactor it by another commit? Show me your implementation of this, please. I want see how you implement something using only strong related changes.

rugabarbo added inline comments.Apr 27 2014, 11:21 AM

src/applications/daemon/management/PhabricatorDaemonManagementStatusWorkflow.php
6	I can strictly follow this article: https://secure.phabricator.com/book/phabflavor/article/writing_reviewable_code/ ...and I can divide my commit into: implementation refactoring But this commit is too small and I can't find reasons for divide it.

@epriestley, what are you think about this?
Should I really divide this commit into different commits?

I can do it.
But I think this commit is reviewable (whitout dividing).

This all seems pretty closely related to showing custom args to me. I don't think it's doing too much stuff.

Instead of reading the arguments out of the database, though, I think we should just store them in the pidfile. In PhutilDaemonOverseer::__construct() (in libphutil), we write a pidfile, near line 150:

if ($this->phddir) {
  $desc = array(
    'name'            => $this->daemon,
    'pid'             => getmypid(),
    'start'           => time(),
  );
  Filesystem::writeFile(
    $this->phddir.'/daemon.'.getmypid(),
    json_encode($desc));
}

I think you can do something like add:

'argv' => $original_argv,

...to this dictionary to get the argv in the pidfile too.

Then, when the file is read in PhabricatorDaemonManagementWorkflow->loadRunningDaemons() (in phabricator), the data will be right there and you won't have to query the database to get it. I think this will be a bit simpler, and will work in more cases (e.g., if the daemon does not have access to the database).

@epriestley, thanks for your advice.
I will try do it.

rugabarbo abandoned this revision.May 17 2014, 8:25 AM