Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cts): add client test for retry strategy #2633

Merged
merged 44 commits into from
Feb 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
3594a6c
feat(cts): add client test for retry strategy
millotp Jan 29, 2024
d7b8935
bit more tricky for java
millotp Jan 29, 2024
72383b8
revert docker
millotp Jan 30, 2024
f145701
progress for go
millotp Jan 30, 2024
0e4f56e
fix java
millotp Jan 30, 2024
1cace3a
works for dart
millotp Jan 31, 2024
80a320c
Merge branch 'main' into feat/retry-test
millotp Jan 31, 2024
ef89edf
undo const
millotp Jan 31, 2024
89a511c
easy ruby
millotp Feb 1, 2024
dfe1135
actually yarn.lock is needed
millotp Feb 1, 2024
d863cae
Merge branch 'main' into feat/retry-test
millotp Feb 11, 2024
042ad82
chsarp not working
millotp Feb 11, 2024
58142e1
python is flaky
millotp Feb 11, 2024
2bd5247
Merge branch 'main' into feat/retry-test
millotp Feb 11, 2024
e627569
fix dart'
millotp Feb 11, 2024
667d469
fix csharp
millotp Feb 11, 2024
8625bb4
I don't understand kotlin
millotp Feb 12, 2024
6cbf32f
php is too hard
millotp Feb 12, 2024
bc978ff
kotlin is working
millotp Feb 12, 2024
7601180
python is working
millotp Feb 12, 2024
6a47dad
Merge branch 'main' into feat/retry-test
millotp Feb 12, 2024
89b7ab4
feat(swift): handle retry strat in client tests (#2643)
Fluf22 Feb 12, 2024
8aa5b27
scala ftw
millotp Feb 12, 2024
49f8623
Merge branch 'main' into feat/retry-test
damcou Feb 13, 2024
b45cbc9
add configurable scheme and port
damcou Feb 13, 2024
b8e4833
add configurable scheme and port
damcou Feb 13, 2024
b46bcc9
add configurable scheme and port
damcou Feb 13, 2024
8d5a419
add full hosts logic
damcou Feb 13, 2024
0aa6b22
remove useless methods
damcou Feb 14, 2024
f502718
update test for php
damcou Feb 14, 2024
ccc6a04
php
millotp Feb 14, 2024
6df79be
Merge branch 'main' into feat/retry-test
millotp Feb 14, 2024
bb7ad1f
fix
millotp Feb 14, 2024
1f13a0d
composer update ?
millotp Feb 14, 2024
dd2c423
composer update on demand
millotp Feb 14, 2024
1f9aeaa
revert composer
millotp Feb 14, 2024
e9f9ce5
shuffle only algolia clusters
damcou Feb 14, 2024
33be0c9
unused import
millotp Feb 14, 2024
aec6b7c
bump cache
millotp Feb 14, 2024
93ec181
explicit port when possible
millotp Feb 14, 2024
5e736f2
forgot python
millotp Feb 14, 2024
f248bc8
fix java
millotp Feb 14, 2024
6fd9084
fix kotlin?
millotp Feb 14, 2024
c25519d
fix ruby
millotp Feb 14, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/.cache_version
Original file line number Diff line number Diff line change
@@ -1 +1 @@
1.0.11
1.0.13
Original file line number Diff line number Diff line change
Expand Up @@ -3,30 +3,43 @@ final class Host {
/// Host url.
final String url;

// url port
final int? port;

/// Host protocol (i.e. `http`, `https`)
final String scheme;

/// Whether this host should be used for [CallType.read] or [CallType.write] requests.
final CallType? callType;

/// Constructs a [Host] instance with the provided parameters.
const Host({required this.url, this.scheme = 'https', this.callType});
const Host({required this.url, this.port, this.scheme = 'https', this.callType});

factory Host.create({required String url, String scheme = 'https', CallType? callType}) {
if (url.contains(':')) {
final parts = url.split(':');
return Host(url: parts[0], port: int.parse(parts[1]), scheme: scheme, callType: callType);
}

return Host(url: url, scheme: scheme, callType: callType);
}

@override
bool operator ==(Object other) =>
identical(this, other) ||
other is Host &&
runtimeType == other.runtimeType &&
url == other.url &&
port == other.port &&
scheme == other.scheme &&
callType == other.callType;

@override
int get hashCode => url.hashCode ^ scheme.hashCode ^ callType.hashCode;
int get hashCode => url.hashCode ^ (port ?? 1) ^ scheme.hashCode ^ callType.hashCode;

@override
String toString() {
return 'Host{url: $url, scheme: $scheme, callType: $callType}';
return 'Host{url: $url, port: $port, scheme: $scheme, callType: $callType}';
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ class DioRequester implements Requester {
Uri uri = Uri(
scheme: request.host.scheme,
host: request.host.url,
port: request.host.port,
path: request.path,
);
if (request.queryParameters.isNotEmpty) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
public final class Host {

private final String url;
private final int port;

private final Set<CallType> callTypes;

Expand All @@ -16,7 +17,12 @@ public Host(@Nonnull String url, @Nonnull Set<CallType> callType) {
}

public Host(String url, Set<CallType> callType, String scheme) {
this(url, callType, scheme, -1);
}

public Host(String url, Set<CallType> callType, String scheme, int port) {
this.url = url;
this.port = port;
this.callTypes = callType;
this.scheme = scheme;
}
Expand All @@ -26,6 +32,10 @@ public String getUrl() {
return url;
}

public int getPort() {
return port;
}

@Nonnull
public Set<CallType> getCallTypes() {
return callTypes;
Expand All @@ -44,13 +54,15 @@ public boolean equals(Object o) {
Host host = (Host) o;

if (!url.equals(host.url)) return false;
if (port != host.port) return false;
if (!callTypes.equals(host.callTypes)) return false;
return scheme.equals(host.scheme);
}

@Override
public int hashCode() {
int result = url.hashCode();
result = 31 * result + port;
result = 31 * result + callTypes.hashCode();
result = 31 * result + scheme.hashCode();
return result;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ public String getHost() {
return host.getUrl();
}

public int getPort() {
return host.getPort();
}

public String getScheme() {
return this.host.getScheme();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,11 @@ public Response intercept(@Nonnull Chain chain) {
/** Processes the request for a given host. */
@Nonnull
private Response processRequest(@Nonnull Chain chain, @Nonnull Request request, StatefulHost host) throws IOException {
HttpUrl newUrl = request.url().newBuilder().scheme(host.getScheme()).host(host.getHost()).build();
HttpUrl.Builder urlBuilder = request.url().newBuilder().scheme(host.getScheme()).host(host.getHost());
if (host.getPort() != -1) {
urlBuilder.port(host.getPort());
}
HttpUrl newUrl = urlBuilder.build();
Request newRequest = request.newBuilder().url(newUrl).build();
chain.withConnectTimeout(chain.connectTimeoutMillis() * (host.getRetryCount() + 1), TimeUnit.MILLISECONDS);
Response response = chain.proceed(newRequest);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export function serializeUrl(
queryParameters: QueryParameters
): string {
const queryParametersAsString = serializeQueryParameters(queryParameters);
let url = `${host.protocol}://${host.url}/${
let url = `${host.protocol}://${host.url}${host.port ? `:${host.port}` : ''}/${
path.charAt(0) === '/' ? path.substr(1) : path
}`;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ export type Host = {
* The protocol of the host URL.
*/
protocol: 'http' | 'https';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why we have a mix of scheme and protocol in the clients

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's from the legacy, I can rename everything if you want, but I have no preference

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk the real definition of both I guess there's some overlap? to me scheme is http or https and protocol is how we transfer data, which is defined by the scheme?

idk idk up to you

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer scheme anyway

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to many idk ahah

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahahah that's my state of mind


/**
* The port of the host URL.
*/
port?: number;
};

export type StatefulHost = Host & {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,6 @@ package com.algolia.client.configuration
public data class Host(
public val url: String,
public val callType: CallType? = null,
public val protocol: String = "https",
public val port: Int? = null,
)
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,11 @@ public class KtorRequester(
val requestBuilder = httpRequestBuilderOf(requestConfig, requestOptions)

for (host in hosts) {
requestBuilder.url.protocol = URLProtocol.createOrDefault(host.protocol)
requestBuilder.url.host = host.url
if (host.port != null) {
requestBuilder.url.port = host.port!!
}
requestBuilder.setTimeout(requestOptions, callType, host)
try {
val response = httpClient.request(requestBuilder)
Expand Down Expand Up @@ -132,8 +136,6 @@ public class KtorRequester(
): HttpRequestBuilder {
return HttpRequestBuilder().apply {
url {
protocol = URLProtocol.HTTPS
port = URLProtocol.HTTPS.defaultPort
pathSegments = requestConfig.path
}
method = requestConfig.method.ktorHttpMethod
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,12 @@ import com.algolia.client.configuration.Host
internal data class RetryableHost(
private val host: Host,
) {

val url
get() = host.url
val protocol
get() = host.protocol
val port
get() = host.port
val callType
get() = host.callType

Expand Down
9 changes: 0 additions & 9 deletions clients/algoliasearch-client-php/lib/Http/Psr7/Uri.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,6 @@ class Uri implements UriInterface
private static $defaultPorts = [
'http' => 80,
'https' => 443,
'ftp' => 21,
'gopher' => 70,
'nntp' => 119,
'news' => 119,
'telnet' => 23,
'tn3270' => 23,
'imap' => 143,
'pop' => 110,
'ldap' => 389,
];

private static $charUnreserved = 'a-zA-Z0-9_\-\.~';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ final class ApiWrapper implements ApiWrapperInterface
*/
private $clusterHosts;

/**
* @var Configuration
*/
private $config;

/**
* @var RequestOptionsFactory
*/
Expand All @@ -57,6 +62,7 @@ public function __construct(
) {
$this->http = $http;
$this->clusterHosts = $clusterHosts;
$this->config = $config;
$this->requestOptionsFactory =
$RqstOptsFactory ?: new RequestOptionsFactory($config);
$this->logger = $logger ?: Algolia::getLogger();
Expand Down Expand Up @@ -148,7 +154,16 @@ private function request(

$retry = 1;
foreach ($hosts as $host) {
$uri = $uri->withHost($host);
if ($this->config->getHasFullHosts()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so IIUC the user has to explicitly call setFullHost to make that work right? if that's the case we should provide some inlined doc to at least explain what we expect

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if they want to specify a different scheme/port, they need to use setFullHosts() , I guess this would require some explanation indeed ...

$host = explode(':', $host);
$uri = $uri->withHost(trim($host[1], '/'))
->withScheme($host[0])
->withPort($host[2])
;
} else {
$uri = $uri->withHost($host);
}

$request = null;
$logParams['retryNumber'] = $retry;
$logParams['host'] = (string) $uri;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public static function createFromAppId($applicationId)
$read[$applicationId.'-dsn.algolia.net'] = 10;
$write[$applicationId.'.algolia.net'] = 10;

return self::create($read, $write);
return self::create($read, $write)->shuffle();
}

public static function createFromCache($cacheKey)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ final class HostCollection
public function __construct(array $hosts)
{
$this->hosts = $hosts;

$this->shuffle();
}

public static function create(array $urlsWithPriority)
Expand Down
17 changes: 12 additions & 5 deletions clients/algoliasearch-client-python/algoliasearch/http/hosts.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,16 @@ class Host:
TTL = 300.0

def __init__(
self, url: str, priority: Optional[int] = 0, accept: Optional[int] = None
self,
url: str,
scheme: str = "https",
port: Optional[int] = None,
priority: Optional[int] = 0,
accept: Optional[int] = None,
) -> None:
self.url = url
self.scheme = scheme
self.port = port
self.priority = cast(int, priority)
self.accept = (CallType.WRITE | CallType.READ) if accept is None else accept

Expand All @@ -21,15 +28,15 @@ def reset(self) -> None:


class HostsCollection:
def __init__(self, hosts: List[Host]) -> None:
def __init__(self, hosts: List[Host], reorder_hosts=False) -> None:
self._hosts = hosts

for host in self._hosts:
host.reset()

shuffle(self._hosts)

self._hosts = sorted(self._hosts, key=lambda x: x.priority, reverse=True)
if reorder_hosts:
shuffle(self._hosts)
self._hosts = sorted(self._hosts, key=lambda x: x.priority, reverse=True)

def read(self) -> List[Host]:
return [host for host in self._hosts if host.accept & CallType.READ]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ async def request(
)

for host in self._retry_strategy.valid_hosts(self._hosts):
url = "https://{}{}".format(host.url, path)
url = "{}://{}{}".format(host.scheme, host.url + (":{}".format(host.port) if host.port else ""), path)

proxy = None
if url.startswith("https"):
Expand All @@ -102,7 +102,7 @@ async def request(
proxy = self._config.proxies.get("http")

try:
async with timeout(self._timeout):
async with timeout(self._timeout / 1000):
resp = await self._session.request(
method=verb,
url=url,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ def send_request(host, method, path, body, query_params, headers, timeout, conne
# @return [Faraday::Connection]
#
def connection(host)
@connections[host.url] ||= Faraday.new(build_url(host)) do |f|
@connections[build_url(host)] ||= Faraday.new(build_url(host)) do |f|
f.adapter @adapter.to_sym
end
end
Expand All @@ -68,7 +68,7 @@ def connection(host)
# @return [String]
#
def build_url(host)
host.protocol + host.url
host.protocol + host.url + (host.port.nil? ? '' : ":#{host.port}")
end

# Convert query_params to a full query string
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ module Transport
class StatefulHost
include CallType

attr_reader :url, :protocol, :accept
attr_reader :url, :protocol, :accept, :port
attr_accessor :last_use, :retry_count, :up

# @param url [String] host url
Expand All @@ -15,6 +15,7 @@ class StatefulHost
def initialize(url, opts = {})
@url = url
@protocol = opts[:protocol] || 'https://'
@port = opts[:port]
@accept = opts[:accept] || (READ | WRITE)
@last_use = opts[:last_use] || Time.now.utc
@retry_count = opts[:retry_count] || 0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,12 @@ def request(call_type, method, path, body, opts = {})
outcome = @retry_strategy.decide(host, http_response_code: response.status, is_timed_out: response.has_timed_out, network_failure: response.network_failure)
if outcome == FAILURE
decoded_error = JSON.parse(response.error, :symbolize_names => true)
raise AlgoliaHttpError.new(decoded_error[:status], decoded_error[:message])
raise Algolia::AlgoliaHttpError.new(decoded_error[:status], decoded_error[:message])
end
return response unless outcome == RETRY
end

raise Algolia::AlgoliaUnreachableHostError('Unreachable hosts')
raise Algolia::AlgoliaUnreachableHostError.new('Unreachable hosts')
end

private
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,7 @@ package algoliasearch.config
* Whether this host should be used for read and/or write requests.
* @param scheme
* Host protocol (i.e. `http`, `https`)
* @param port
* Host port
*/
case class Host(url: String, callTypes: Set[CallType], scheme: String = "https")
case class Host(url: String, callTypes: Set[CallType], scheme: String = "https", port: Option[Int] = None)
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ private[algoliasearch] class StatefulHost(private val host: Host) {

def getScheme: String = host.scheme

def getPort: Option[Int] = host.port

def isUp: Boolean = up

def getRetryCount: Int = retryCount
Expand Down
Loading
Loading