Page MenuHomePhabricator
Differential D17384 Diff 42163 src/applications/config/controller/PhabricatorConfigClusterSearchController.php

Changeset View

Standalone View

View Options

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(
epriestleyUnsubmitted
Done
Inline Actions

Maybe this logic should go in getHostRefs()? Feels a little weird to make callers do it...

epriestley: Maybe this logic should go in `getHostRefs()`? Feels a little weird to make callers do it...
20after4AuthorUnsubmitted
Done
Inline Actions

agreed.

20after4: agreed.
array(pht('Type')),
array_keys($service->getStatusViewColumns()),
array(pht('Status')));
epriestleyUnsubmitted
Done
Inline Actions

Since 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…
20after4AuthorUnsubmitted
Done
Inline Actions

getAllServices 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();
epriestleyUnsubmitted
Done
Inline Actions

This 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(
epriestleyUnsubmitted
Done
Inline Actions

I 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']),
' ',
epriestleyUnsubmitted
Done
Inline Actions

This 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;
epriestleyUnsubmitted
Done
Inline Actions

(Maybe put a "readable" column in here too?)

epriestley: (Maybe put a "readable" column in here too?)
20after4AuthorUnsubmitted
Done
Inline Actions

I 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);
epriestleyUnsubmitted
Done
Inline Actions

Unconventional 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;
}
private function renderYes($info) {
return array(
id(new PHUIIconView())->setIcon('fa-check', 'green'),
' ',
$info,
);
}
epriestleyUnsubmitted
Done
Inline Actions

Remove, unused?

epriestley: Remove, unused?
private function renderNo($info) {
return array(
id(new PHUIIconView())->setIcon('fa-times-circle', 'red'),
' ',
$info,
);
}
epriestleyUnsubmitted
Done
Inline Actions

Prefer checkIcon().

epriestley: Prefer `checkIcon()`.
epriestleyUnsubmitted
Done
Inline Actions

Remove, unused?

epriestley: Remove, unused?
private function renderInfo($info) {
return array(
id(new PHUIIconView())->setIcon('fa-info-circle', 'grey'),
epriestleyUnsubmitted
Done
Inline Actions

Remove, no callsites?

epriestley: Remove, no callsites?
' ',
$info,
);
}
epriestleyUnsubmitted
Done
Inline Actions

Remove, unused?

epriestley: Remove, unused?
}
epriestleyUnsubmitted
Done
Inline Actions

No callers now?

epriestley: No callers now?
Phabricator · Built by Phacility