Changeset View
Standalone View
src/applications/config/controller/PhabricatorConfigClusterSearchController.php
- This file was added.
<?php | |||||
final class PhabricatorConfigClusterSearchController | |||||
extends PhabricatorConfigController { | |||||
public function handleRequest(AphrontRequest $request) { | |||||
$nav = $this->buildSideNavView(); | |||||
$nav->selectFilter('cluster/search/'); | |||||
$title = pht('Cluster Search'); | |||||
$doc_href = PhabricatorEnv::getDoclink('Cluster: Search'); | |||||
$header = id(new PHUIHeaderView()) | |||||
->setHeader($title) | |||||
->setProfileHeader(true) | |||||
->addActionLink( | |||||
id(new PHUIButtonView()) | |||||
->setIcon('fa-book') | |||||
->setHref($doc_href) | |||||
->setTag('a') | |||||
->setText(pht('Documentation'))); | |||||
$crumbs = $this | |||||
->buildApplicationCrumbs($nav) | |||||
->addTextCrumb($title) | |||||
->setBorder(true); | |||||
$search_status = $this->buildClusterSearchStatus(); | |||||
$content = id(new PhabricatorConfigPageView()) | |||||
->setHeader($header) | |||||
->setContent($search_status); | |||||
return $this->newPage() | |||||
->setTitle($title) | |||||
->setCrumbs($crumbs) | |||||
->setNavigation($nav) | |||||
->appendChild($content) | |||||
->addClass('white-background'); | |||||
} | |||||
private function buildClusterSearchStatus() { | |||||
$viewer = $this->getViewer(); | |||||
$services = PhabricatorSearchService::getAllServices(); | |||||
Javelin::initBehavior('phabricator-tooltips'); | |||||
$view = array(); | |||||
foreach ($services as $service) { | |||||
$view[] = $this->renderStatusView($service); | |||||
} | |||||
return $view; | |||||
} | |||||
private function renderStatusView($service) { | |||||
$head = array_merge( | |||||
epriestley: Maybe this logic should go in `getHostRefs()`? Feels a little weird to make callers do it... | |||||
Done Inline Actionsagreed. 20after4: agreed. | |||||
array(pht('Type')), | |||||
array_keys($service->getStatusViewColumns()), | |||||
array(pht('Status'))); | |||||
Done Inline ActionsSince we call a method on $engine explicitly later ($engine->getEnginePriority()) I think we'll fatal if getEngine() throws (or maybe use the wrong $engine). Probably $engine = null first, then test for $engine when it gets used later? Test case for this is probably something like "configure a service with a made-up type like 'quack'", although maybe that doesn't get this far. epriestley: Since we call a method on `$engine` explicitly later (`$engine->getEnginePriority()`) I think… | |||||
Done Inline ActionsgetAllServices should be ensuring that everything gets constructed properly so that we can always get a reference to the engine. 20after4: `getAllServices` should be ensuring that everything gets constructed properly so that we can… | |||||
$rows = array(); | |||||
$status_map = PhabricatorSearchService::getConnectionStatusMap(); | |||||
foreach ($service->getHosts() as $host) { | |||||
$reachable = false; | |||||
try { | |||||
$engine = $host->getEngine(); | |||||
$reachable = $engine->indexExists(); | |||||
} catch (Exception $ex) { | |||||
$reachable = false; | |||||
} | |||||
$host->didHealthCheck($reachable); | |||||
try { | |||||
$status = $host->getConnectionStatus(); | |||||
$status = idx($status_map, $status, array()); | |||||
$stats = $engine->getIndexStats(); | |||||
} catch (Exception $ex) { | |||||
$status['icon'] = 'fa-times'; | |||||
$status['label'] = pht('Connection Error'); | |||||
$status['color'] = 'red'; | |||||
$stats = array(); | |||||
} | |||||
$stats_view = $this->renderIndexStats($stats); | |||||
$type_icon = 'fa-search sky'; | |||||
$type_tip = $host->getDisplayName(); | |||||
Done Inline ActionsThis is purely a style nitpick, buuut: Mostly for codebase consistency, avoid {$var['index']} and {$obj->method()} and similar fancy expressions in strings. There's a weak technical reason for this: XHPAST doesn't do a very good job of them until T8049 is resolved. (There's another weak argument that stack traces can be a bit harder to hunt down at times.) But even if the XHPAST was resolved, I think we'd still discourage this stuff from a style viewpoint. This is legitimate in PHP: "{$v[""]}" ...and it "works": $ cat php.php <?php $v = array("" => "quack"); echo "{$v[""]}"; $ php -f php.php quack ...but I think this is generally better written as: $status['icon'].' '.$status['color'] And, purely from a consistency standpoint, we avoid complex string expressions elsewhere in the codebase. This particular case is also sort of arguably an API problem. This should perhaps be separate setIcon() and setColor() calls, or setIcon($icon, $color = null), but we ended up not being completely consistent about that. We do $icon, $color = null about half the time, I think. epriestley: This is purely a style nitpick, buuut:
Mostly for codebase consistency, avoid `{$var… | |||||
$type_icon = id(new PHUIIconView()) | |||||
->setIcon($type_icon); | |||||
$status_view = array( | |||||
Done Inline ActionsI think we can just remove the "Engine Priority" thing, since it's just determined by the service order in cluster.search now. Before, there was no inherent order to elastic vs mysql so we needed an explicit order, but cluster.search now provides an explicit order. epriestley: I think we can just remove the "Engine Priority" thing, since it's just determined by the… | |||||
id(new PHUIIconView())->setIcon($status['icon'].' '.$status['color']), | |||||
' ', | |||||
Done Inline ActionsThis icon has both a tooltip and a label, maybe get rid of one? Prefer this: array($x, ' ', $y); ...over this: array($x, ' '.$y); The former should always work, the latter will fail if $y later becomes a tag (for example, someone adds a <strong> around it or whatever). epriestley: This icon has both a tooltip and a label, maybe get rid of one?
Prefer this:
```
array($x, '… | |||||
$status['label'], | |||||
); | |||||
$row = array(array($type_icon, ' ', $type_tip)); | |||||
$row = array_merge($row, array_values( | |||||
$host->getStatusViewColumns())); | |||||
$row[] = $status_view; | |||||
$rows[] = $row; | |||||
Done Inline Actions(Maybe put a "readable" column in here too?) epriestley: (Maybe put a "readable" column in here too?) | |||||
Done Inline ActionsI went with "Roles" instead of separate 'readable' and 'writable' columns. 20after4: I went with "Roles" instead of separate 'readable' and 'writable' columns. | |||||
} | |||||
$table = id(new AphrontTableView($rows)) | |||||
->setNoDataString(pht('No search servers are configured.')) | |||||
->setHeaders($head); | |||||
Done Inline ActionsUnconventional indent formatting. epriestley: Unconventional indent formatting. | |||||
return id(new PHUIObjectBoxView()) | |||||
->setHeaderText($service->getDisplayName()) | |||||
->addPropertyList($stats_view) | |||||
->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) | |||||
->setTable($table); | |||||
} | |||||
private function renderIndexStats($stats) { | |||||
$view = id(new PHUIPropertyListView()); | |||||
if ($stats !== false) { | |||||
foreach ($stats as $label => $val) { | |||||
$view->addProperty($label, $val); | |||||
} | |||||
} | |||||
return $view; | |||||
} | |||||
} | |||||
Done Inline ActionsPrefer checkIcon(). epriestley: Prefer `checkIcon()`. | |||||
Done Inline ActionsNo callers now? epriestley: No callers now? | |||||
Done Inline ActionsRemove, no callsites? epriestley: Remove, no callsites? | |||||
Done Inline ActionsRemove, unused? epriestley: Remove, unused? | |||||
Done Inline ActionsRemove, unused? epriestley: Remove, unused? | |||||
Done Inline ActionsRemove, unused? epriestley: Remove, unused? |
Maybe this logic should go in getHostRefs()? Feels a little weird to make callers do it...