Page MenuHomePhabricator

Introduce a new private method `getMultiHttpClient()` to replace `getServiceClient()`
Closed, ResolvedPublic

Description

This method will be responsible for creating the necessary HTTP client (whether single or multi) and will replace getServiceClient which creates a VRS instance. The interface should resemble what is done in getServiceClient() but just that instead of creating an instance of VRS, create an instance of a MultiHttpClient.

Interface should look like:

private function getMultiHttpClient() { ... }

The method should return an instance of MultiHttpClient from our HttpRequestFactory service, where we can call ::run() or ::runMulti() on it depending on the endpoint we're calling/consuming.

NOTE: This method should also take care of the $wgVirtualRestConfig and $wgMathConcurrentReqs settings. This is so that, the client can be able to run batch, single and parallel requests depending on the configurations.
NOTE: also note that $wgVirtualRestConfig's parsoidCompat is always set to false when we're in RB mode and even RestbaseVirtualRESTService::__construct() sets this to false as well by default (meaning Math/Mathoid doesn't call Parsoid by default). We need to also pass in the value of $wgVirtualRestConfig['modules']['restbase'] to the MultiHttpClient.

Event Timeline

DAlangi_WMF created this task.
DAlangi_WMF renamed this task from Introduce a new private method `getHttpRequestClient()` to replace `getServiceClient()` to Introduce a new private method `getMultiHttpClient()` to replace `getServiceClient()`.Apr 25 2023, 11:16 AM
DAlangi_WMF updated the task description. (Show Details)

Some hints from looking through the code:

If $wgVirtualRestConfig['modules']['restbase'] is set, MathRestbaseInterface::getUrl() should apply the URL in $wgVirtualRestConfig['modules']['restbase']['url'] as a prefix to the string it returns:

// old code:
return "/mathoid/local/v1/$path";

// new code:
return $wgVirtualRestConfig['modules']['restbase']['url'] . "/mathoid/local/v1/$path";

A somewhat incomplete thought:

The calls to $serviceClient->run and $serviceClient->runMulti in MathRestbaseInterface should be replaced by calls to MultiHttpClient::run and MultiHttpClient::runMulti. For reference, VirtualRESTServiceClient::runMulti has the following code:

			foreach ( $this->http->runMulti( $executeReqs, $opts ) as $index => $ranReq ) {
				$doneReqs[$index] = $ranReq;
				unset( $origPending[$index] );
			}

@daniel: In the ::getUrl() method, we always try to access resources from RB internally (by default) which is why we mount paths to URLs but since we're removing usage of VirtualRESTService from Math, we still need to support it but it should not be the default since now we're just making plain HTTP requests?

Maybe Moritz can tell us use cases for accessing RB resources from the inside "only". Does this still happen or can we just kill the internal parameter entirely?

Change 913243 had a related patch set uploaded (by Richika Rana; author: Richika Rana):

[mediawiki/extensions/Math@master] Add getMultiHttpClient function to make HTTP requests to Mathoid.

https://gerrit.wikimedia.org/r/913243

Maybe Moritz can tell us use cases for accessing RB resources from the inside "only". Does this still happen or can we just kill the internal parameter entirely?

Unfortunately, I don't know.

Change 913243 merged by jenkins-bot:

[mediawiki/extensions/Math@master] Add getMultiHttpClient function to make HTTP requests to Mathoid.

https://gerrit.wikimedia.org/r/913243

Change 919353 had a related patch set uploaded (by Daniel Kinzler; author: Richika Rana):

[mediawiki/extensions/Math@master] Use MultiHttpClient instead of VirtualRESTService.

https://gerrit.wikimedia.org/r/919353

Thanks for working on this @R_Rana.

Change 919309 had a related patch set uploaded (by Daniel Kinzler; author: Subramanya Sastry):

[mediawiki/extensions/Math@master] Revert "Revert "Add getMultiHttpClient function to make HTTP requests to Mathoid.""

https://gerrit.wikimedia.org/r/919309

Re-opening - the patch was reverted, we'll re-deploy tomorrow

Change 920230 had a related patch set uploaded (by Daniel Kinzler; author: Subramanya Sastry):

[mediawiki/extensions/Math@wmf/1.41.0-wmf.9] Revert "Revert "Add getMultiHttpClient function to make HTTP requests to Mathoid.""

https://gerrit.wikimedia.org/r/920230

Change 920231 had a related patch set uploaded (by Daniel Kinzler; author: Richika Rana):

[mediawiki/extensions/Math@wmf/1.41.0-wmf.9] Use MultiHttpClient instead of VirtualRESTService.

https://gerrit.wikimedia.org/r/920231

Change 920230 merged by jenkins-bot:

[mediawiki/extensions/Math@wmf/1.41.0-wmf.9] Revert "Revert "Add getMultiHttpClient function to make HTTP requests to Mathoid.""

https://gerrit.wikimedia.org/r/920230

Change 920231 merged by jenkins-bot:

[mediawiki/extensions/Math@wmf/1.41.0-wmf.9] Use MultiHttpClient instead of VirtualRESTService.

https://gerrit.wikimedia.org/r/920231

Mentioned in SAL (#wikimedia-operations) [2023-05-17T13:06:22Z] <daniel@deploy1002> Started scap: Backport for [[gerrit:920230|Revert "Revert "Add getMultiHttpClient function to make HTTP requests to Mathoid."" (T335347)]], [[gerrit:920231|Use MultiHttpClient instead of VirtualRESTService. (T335347)]]

Mentioned in SAL (#wikimedia-operations) [2023-05-17T13:07:56Z] <daniel@deploy1002> daniel: Backport for [[gerrit:920230|Revert "Revert "Add getMultiHttpClient function to make HTTP requests to Mathoid."" (T335347)]], [[gerrit:920231|Use MultiHttpClient instead of VirtualRESTService. (T335347)]] synced to the testservers: mwdebug1001.eqiad.wmnet, mwdebug2002.codfw.wmnet, mwdebug2001.codfw.wmnet, mwdebug1002.eqiad.wmnet

Mentioned in SAL (#wikimedia-operations) [2023-05-17T13:18:14Z] <daniel@deploy1002> Finished scap: Backport for [[gerrit:920230|Revert "Revert "Add getMultiHttpClient function to make HTTP requests to Mathoid."" (T335347)]], [[gerrit:920231|Use MultiHttpClient instead of VirtualRESTService. (T335347)]] (duration: 11m 52s)

Change 919309 merged by jenkins-bot:

[mediawiki/extensions/Math@master] Revert "Revert "Add getMultiHttpClient function to make HTTP requests to Mathoid.""

https://gerrit.wikimedia.org/r/919309

Re-opening - the patch was reverted, we'll re-deploy tomorrow

Deployed!

Change 919353 merged by jenkins-bot:

[mediawiki/extensions/Math@master] Use MultiHttpClient instead of VirtualRESTService.

https://gerrit.wikimedia.org/r/919353