Differential D17384 Diff 41928 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 = PhabricatorSearchCluster::getAllServices();
				Javelin::initBehavior('phabricator-tooltips');
				$status_map = PhabricatorDatabaseRef::getConnectionStatusMap();
				$rows = array();
				foreach ($services as $service) {

				$reachable = false;
				try {
				$engine = $service->getEngine();
				$reachable = $engine->indexExists();
				} catch (Exception $ex) {
				$reachable = false;
				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.
				}
				$service->didHealthCheck($reachable);

				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…
				try {
				$status = $service->loadServerStatus();
				$status = idx($status_map, $status, array());
				} catch (Exception $ex) {
				$status['icon'] = 'fa-times';
				$status['label'] = pht('Connection Error');
				$status['color'] = 'red';
				}

				$type_icon = 'fa-search sky';
				$type_tip = $service->getDisplayName();

				$type_icon = id(new PHUIIconView())
				->setIcon($type_icon)
				->addSigil('has-tooltip')
				->setMetadata(
				array(
				'tip' => $type_tip,
				));

				$hosts = $service->getHostRefs();
				if (!count($hosts)) {
				$hosts = array($service);
				}
				foreach ($hosts as $host) {
				$status_view = array(
				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…
				id(new PHUIIconView())->setIcon(
				"{$status['icon']} {$status['color']}"),
				' ',
				$status['label'],
				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…
				);
				$roles = implode(', ', array_keys($host->getRoles()));
				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, '…
				$rows[] = array(
				array($type_icon, ' '.$type_tip),
				$host->getProtocol(),
				$host->getHost(),
				$host->getPort(),
				$status_view,
				$roles,
				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.
				$engine->getEnginePriority(),
				);
				}
				}

				$table = id(new AphrontTableView($rows))
				epriestleyUnsubmitted Done Inline Actions Unconventional indent formatting. epriestley: Unconventional indent formatting.
				->setNoDataString(
				pht('No search servers are configured.'))
				->setHeaders(
				array(
				pht('Type'),
				pht('Protocol'),
				pht('Host'),
				pht('Port'),
				pht('Status'),
				pht('Roles'),
				pht('Priority'),
				null,
				))
				->setColumnClasses(
				array(
				null,
				null,
				null,
				null,
				null,
				null,
				null,
				'wide',
				));

				epriestleyUnsubmitted Done Inline Actions Remove, unused? epriestley: Remove, unused?
				return $table;
				}

				private function checkIcon($check) {
				$icon = $check
				? 'fa-check green'
				: 'fa-times red';
				epriestleyUnsubmitted Done Inline Actions Prefer `checkIcon()`. epriestley: Prefer `checkIcon()`.
				$label = $check
				epriestleyUnsubmitted Done Inline Actions Remove, unused? epriestley: Remove, unused?
				? pht('Yes')
				: pht('No');
				$view = id(new PHUIIconView())
				epriestleyUnsubmitted Done Inline Actions Remove, no callsites? epriestley: Remove, no callsites?
				->setIcon($icon)
				->addSigil('has-tooltip')
				->setMetadata(
				array(
				'tip' => $label,
				epriestleyUnsubmitted Done Inline Actions Remove, unused? epriestley: Remove, unused?
				));
				epriestleyUnsubmitted Done Inline Actions No callers now? epriestley: No callers now?

				return $view;
				}
				}