Differential D17384 Diff 42188 src/applications/config/controller/PhabricatorConfigClusterSearchController.php

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(
				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;
				}

				}
				epriestleyUnsubmitted Done Inline Actions Prefer `checkIcon()`. epriestley: Prefer `checkIcon()`.
				epriestleyUnsubmitted Done Inline Actions No callers now? epriestley: No callers now?
				epriestleyUnsubmitted Done Inline Actions Remove, no callsites? epriestley: Remove, no callsites?
				epriestleyUnsubmitted Done Inline Actions Remove, unused? epriestley: Remove, unused?
				epriestleyUnsubmitted Done Inline Actions Remove, unused? epriestley: Remove, unused?
				epriestleyUnsubmitted Done Inline Actions Remove, unused? epriestley: Remove, unused?