Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T4209: Multiserver / High-Availability Configuration

Commits

Restricted Diffusion Commit
rPf52fbf61170d: Unify the local and global view for `./bin/phd status`.

Summary

Ref T4209. Unifies the local (./bin/phd status) and global (./bin/phd status --all) view into a single table. This generally makes it easy to administer daemons running across multiple hosts.

Depends on D9606.

Test Plan

> sudo ./bin/phd status 
ID Host      PID  Started                 Daemon                               Arguments
38 localhost 2282 Jun 18 2014, 7:52:56 AM PhabricatorRepositoryPullLocalDaemon          
39 localhost 2289 Jun 18 2014, 7:52:57 AM PhabricatorGarbageCollectorDaemon             
40 localhost 2294 Jun 18 2014, 7:52:57 AM PhabricatorTaskmasterDaemon                   
41 localhost 2314 Jun 18 2014, 7:52:58 AM PhabricatorTaskmasterDaemon                   
42 localhost 2319 Jun 18 2014, 7:52:59 AM PhabricatorTaskmasterDaemon                   
43 localhost 2328 Jun 18 2014, 7:53:00 AM PhabricatorTaskmasterDaemon                   
44 localhost 2354 Jun 18 2014, 7:53:08 AM PhabricatorRepositoryPullLocalDaemon X --not Y

Diff Detail

Repository

rP Phabricator

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

joshuaspence updated this revision to Diff 23028.Jun 17 2014, 9:01 PM

joshuaspence retitled this revision from to Unify the local and global view for `./bin/phd status`..

joshuaspence updated this object.

joshuaspence edited the test plan for this revision. (Show Details)

joshuaspence added a reviewer: epriestley.

joshuaspence added a parent revision: D9606: Add a `withoutIDs` method to `PhabricatorDaemonLogQuery`..

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJun 17 2014, 9:01 PM

Herald added subscribers: Korvin, epriestley. · View Herald Transcript

Harbormaster completed remote builds in B1191: Diff 23028.Jun 17 2014, 9:02 PM

Minor fixes
Remove formatting
Fix for possible undefined variable

Harbormaster completed remote builds in B1192: Diff 23029.Jun 17 2014, 9:56 PM

joshuaspence updated this object.Jun 17 2014, 9:57 PM

joshuaspence edited the test plan for this revision. (Show Details)

joshuaspence added a task: T4209: Multiserver / High-Availability Configuration.

epriestley accepted this revision.Jun 17 2014, 10:08 PM

epriestley edited edge metadata.

epriestley added inline comments.

src/applications/daemon/query/PhabricatorDaemonLogQuery.php
28–32 ↗	(On Diff #23029)	(These changes appear elsewhere?)

This revision is now accepted and ready to land.Jun 17 2014, 10:08 PM

joshuaspence added inline comments.Jun 17 2014, 10:09 PM

src/applications/daemon/query/PhabricatorDaemonLogQuery.php
28–32 ↗	(On Diff #23029)	Diffed against `master` instead of D9606.

joshuaspence planned changes to this revision.Jun 17 2014, 10:10 PM

joshuaspence added inline comments.

src/applications/daemon/management/PhabricatorDaemonManagementStatusWorkflow.php
56	I forgot that I hadn't yet implemented this... thoughts on how to tackle this?

epriestley added inline comments.Jun 17 2014, 10:18 PM

src/applications/daemon/management/PhabricatorDaemonManagementStatusWorkflow.php
56	The issue is that the DB version is too verbose? I think either adding a method to PhabricatorDaemonLog like `getInterestingArgv()` and filtering there, or adding a new column and inserting only the interesting stuff are both reasonable approaches. Although it looks like we aren't GC'ing this table, so maybe we should do that before schema mutations. So I guess I slightly favor: Add `getInterestingArgv()` or similar for now and just filter with a regexp. If you want to pursue filtering when the record is inserted eventually, add GC for this table (maybe I'll just shoot you a diff for this). After GC has been in for a while, mutate the table freely. Or, I think you could safely change the code to insert only the interesting arguments, but it seems somewhat valuable to retain the entire original argv.

We only have ~16K rows on secure.phabricator.com and are probably near the high end of all installs (i.e., we run the normal number of daemons but probably restart way more often than most installs do) so if you really want to mutate the table that's probably fine to do today without waiting for the GC to clean things up in the wild.

Add interestingArgv column to phabricator_daemon.daemon_log

This revision is now accepted and ready to land.Jun 17 2014, 10:46 PM

Harbormaster completed remote builds in B1198: Diff 23041.Jun 17 2014, 10:47 PM

joshuaspence requested a review of this revision.Jun 17 2014, 10:47 PM

joshuaspence edited edge metadata.

joshuaspence added inline comments.

resources/sql/autopatches/20140617.daemonlog.sql
1–2 ↗	(On Diff #23041)	Actually, renaming this is a bad idea.

epriestley added inline comments.Jun 17 2014, 10:57 PM

resources/sql/autopatches/20140617.daemonlog.sql
1–2 ↗	(On Diff #23041)	yes plz no rename These patches don't need to apply in order so the new one can be called whatever.

Small fix
interestingArgv can be null

Harbormaster completed remote builds in B1199: Diff 23043.Jun 17 2014, 11:02 PM

Seems fine, modulo one inline, depending on how you do the implementation.

src/applications/daemon/storage/PhabricatorDaemonLog.php
22–24	(Should `interestingArgv` also have JSON serialization?)

joshuaspence added inline comments.Jun 17 2014, 11:05 PM

src/applications/daemon/storage/PhabricatorDaemonLog.php
22–24	Yeah, probably. Will fix.

Changed my mind... interestingArgv shouldn't be null.

Harbormaster completed remote builds in B1200: Diff 23044.Jun 17 2014, 11:09 PM

joshuaspence added a parent revision: D9613: Pass additional arguments to the `PhabricatorDaemonEventListener`..Jun 17 2014, 11:16 PM

loadAvailableDaemons is used in other places

epriestley accepted this revision.Jun 17 2014, 11:24 PM

epriestley edited edge metadata.

This revision is now accepted and ready to land.Jun 17 2014, 11:24 PM

Harbormaster completed remote builds in B1202: Diff 23046.Jun 17 2014, 11:25 PM

Last change, I swear!

Harbormaster completed remote builds in B1203: Diff 23047.Jun 17 2014, 11:33 PM

@epriestley, before I land this... I don't suppose there is a better name than interestingArgv? I'm not a huge fan TBH.

Hmm, explicitArgv?

That'll do. The more terms I read interestingArgv the more I disliked it.

Change interestingArgv to explicitArgv

Just being super cautious (paranoid?) and making sure this still all walks. I'll do a quick test on my dev box too.

Harbormaster completed remote builds in B1210: Diff 23056.Jun 18 2014, 1:21 AM

(Seems to work fine from my tests)

epriestley accepted this revision.Jun 18 2014, 1:30 AM

epriestley edited edge metadata.

epriestley added inline comments.

src/applications/daemon/management/PhabricatorDaemonManagementStatusWorkflow.php
74	Not really relevant, but this `phutil_console_format()` is a bit fluff.
src/infrastructure/daemon/control/PhabricatorDaemonReference.php
15–20	This could be slightly simplified with `phutil_json_decode()`.

This revision is now accepted and ready to land.Jun 18 2014, 1:30 AM

Minor changes as per @epriestley's comments

Harbormaster completed remote builds in B1211: Diff 23058.Jun 18 2014, 1:44 AM

Closed by commit rPf52fbf61170d (authored by @joshuaspence).

Unify the local and global view for `./bin/phd status`.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 23059

resources/sql/autopatches/20140617.daemon.explicit-argv.sql

src/applications/daemon/event/PhabricatorDaemonEventListener.php

src/applications/daemon/management/PhabricatorDaemonManagementStatusWorkflow.php

src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php

src/applications/daemon/storage/PhabricatorDaemonLog.php

src/infrastructure/daemon/control/PhabricatorDaemonReference.php

Unify the local and global view for `./bin/phd status`.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 23059

resources/sql/autopatches/20140617.daemon.explicit-argv.sql

src/applications/daemon/event/PhabricatorDaemonEventListener.php

src/applications/daemon/management/PhabricatorDaemonManagementStatusWorkflow.php

src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php

src/applications/daemon/storage/PhabricatorDaemonLog.php

src/infrastructure/daemon/control/PhabricatorDaemonReference.php

Unify the local and global view for `./bin/phd status`.
ClosedPublic
Actions

Revision Contents
Changeset List