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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Add retry on NoHttpResponseException #1721

Merged
merged 5 commits into from
Jul 9, 2024
Merged

Add retry on NoHttpResponseException #1721

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
372322a
Added retry on no reponse exception
chernser Jul 5, 2024
1ec1c9a
added check for the option to enable retry
chernser Jul 5, 2024
29cf395
no retry by default because of risk
chernser Jul 5, 2024
41b3e29
added retry on 503
chernser Jul 6, 2024
845e76d
Merge branch 'main' into fix_stale_connection_issue
chernser Jul 9, 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
41 changes: 33 additions & 8 deletions ...khouse-http-client/src/main/java/com/clickhouse/client/http/ApacheHttpConnectionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import com.clickhouse.client.AbstractSocketClient;
import com.clickhouse.client.ClickHouseClient;
import com.clickhouse.client.ClickHouseConfig;
import com.clickhouse.client.ClickHouseException;
import com.clickhouse.client.ClickHouseNode;
import com.clickhouse.client.ClickHouseRequest;
import com.clickhouse.client.ClickHouseSocketFactory;
Expand Down Expand Up @@ -31,9 +30,11 @@
import org.apache.hc.client5.http.socket.PlainConnectionSocketFactory;
import org.apache.hc.client5.http.ssl.DefaultHostnameVerifier;
import org.apache.hc.client5.http.ssl.SSLConnectionSocketFactory;
import org.apache.hc.core5.http.ConnectionClosedException;
import org.apache.hc.core5.http.Header;
import org.apache.hc.core5.http.HttpHost;
import org.apache.hc.core5.http.HttpRequest;
import org.apache.hc.core5.http.NoHttpResponseException;
import org.apache.hc.core5.http.config.Registry;
import org.apache.hc.core5.http.config.RegistryBuilder;
import org.apache.hc.core5.http.io.SocketConfig;
Expand All @@ -45,7 +46,6 @@

import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLException;

import java.io.BufferedReader;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
Expand All @@ -58,7 +58,6 @@
import java.net.HttpURLConnection;
import java.net.InetSocketAddress;
import java.net.Socket;
import java.net.StandardSocketOptions;
import java.nio.charset.StandardCharsets;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -251,11 +250,35 @@ protected ClickHouseHttpResponse post(ClickHouseConfig config, String sql, Click
ClickHouseHttpEntity postBody = new ClickHouseHttpEntity(config, contentType, contentEncoding, boundary,
sql, data, tables);
post.setEntity(postBody);
CloseableHttpResponse response;
try {
response = client.execute(post);
} catch (IOException e) {
throw new ConnectException(ClickHouseUtils.format("HTTP request failed: %s", e.getMessage()));
CloseableHttpResponse response = null;

int retryAttempts = config.getBoolOption(ClickHouseHttpOption.AHC_RETRY_ON_FAILURE) ? 2 : 1;
for (int attempt = 0; attempt < retryAttempts; attempt++) {
boolean isLastAttempt = attempt == retryAttempts - 1;
log.debug("HTTP request attempt " + attempt);
try {
response = client.execute(post);

if (!isLastAttempt && (response.getCode() == HttpURLConnection.HTTP_UNAVAILABLE)) {
log.debug("HTTP request failed with status code 503, retrying...");
continue;
}

break;
} catch (NoHttpResponseException | ConnectionClosedException e) {
if (isLastAttempt) {
throw new ConnectException(e.getMessage());
} else {
continue;
}
} catch (IOException e) {
log.error("HTTP request failed", e);
throw new ConnectException(e.getMessage());
}
}
if (response == null) {
// Should not happen but needed for compiler
throw new ConnectException("HTTP request failed");
}

checkResponse(config, response);
Expand Down Expand Up @@ -376,6 +399,8 @@ public HttpConnectionManager(Registry<ConnectionSocketFactory> socketFactory, Cl

ConnectionConfig connConfig = ConnectionConfig.custom()
.setConnectTimeout(Timeout.of(config.getConnectionTimeout(), TimeUnit.MILLISECONDS))
.setTimeToLive(20, TimeUnit.SECONDS)
.setValidateAfterInactivity(10, TimeUnit.SECONDS)
.build();
setDefaultConnectionConfig(connConfig);

Expand Down
18 changes: 17 additions & 1 deletion ...use-http-client/src/main/java/com/clickhouse/client/http/config/ClickHouseHttpOption.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import com.clickhouse.data.ClickHouseChecker;

import java.io.Serializable;
import java.net.UnknownHostException;

/**
* Http client options.
Expand Down Expand Up @@ -64,7 +65,22 @@ public enum ClickHouseHttpOption implements ClickHouseOption {
* Only one role can be set at a time.
*/
REMEMBER_LAST_SET_ROLES("remember_last_set_roles", false,
"Whether to remember last set role and send them in every next requests as query parameters.");
"Whether to remember last set role and send them in every next requests as query parameters."),

/**
* Whether to retry on failure with AsyncHttpClient. Failure includes some 'critical' IO exceptions:
* <ul>
* <li>{@code org.apache.hc.core5.http.ConnectionClosedException}</li>
* <li>{@code org.apache.hc.core5.http.NoHttpResponseException}</li>
* </ul>
*
* And next status codes:
* <ul>
* <li>{@code 503 Service Unavailable}</li>
* </ul>
*/
AHC_RETRY_ON_FAILURE("ahc_retry_on_failure", false, "Whether to retry on failure with AsyncHttpClient.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change the name of the property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sure. But let me explain why I've chosen this one - implementation works only for Apache HC and only for old client.
In the new client I would name it http.retry_on_failure meaning it will be supported for every http implementation we ever have.

;

private final String key;
private final Serializable defaultValue;
Expand Down
63 changes: 63 additions & 0 deletions ...se-http-client/src/test/java/com/clickhouse/client/http/ApacheHttpConnectionImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,15 @@
import java.util.concurrent.atomic.AtomicBoolean;

import com.github.tomakehurst.wiremock.WireMockServer;
import com.github.tomakehurst.wiremock.admin.model.ScenarioState;
import com.github.tomakehurst.wiremock.client.WireMock;
import com.github.tomakehurst.wiremock.http.Fault;
import com.github.tomakehurst.wiremock.stubbing.Scenario;
import com.github.tomakehurst.wiremock.stubbing.StubMapping;
import org.apache.hc.client5.http.socket.PlainConnectionSocketFactory;
import org.apache.hc.core5.http.HttpStatus;
import org.testng.Assert;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;

public class ApacheHttpConnectionImplTest extends ClickHouseHttpClientTest {
Expand Down Expand Up @@ -139,9 +144,67 @@ public void testFailureWhileRequest() {
httpClient.executeAndWait(request);
} catch (ClickHouseException e) {
Assert.assertEquals(e.getErrorCode(), ClickHouseException.ERROR_NETWORK);
return;
}

Assert.fail("Should throw exception");
} finally {
faultyServer.stop();
}
}

@Test(groups = {"unit"}, dataProvider = "retryOnFailureProvider")
public void testRetryOnFailure(StubMapping failureStub) {
faultyServer = new WireMockServer(9090);
faultyServer.start();
try {
faultyServer.addStubMapping(failureStub);
faultyServer.addStubMapping(WireMock.post(WireMock.anyUrl())
.withRequestBody(WireMock.equalTo("SELECT 1"))
.inScenario("Retry")
.whenScenarioStateIs("Failed")
.willReturn(WireMock.aResponse()
.withHeader("X-ClickHouse-Summary",
"{ \"read_bytes\": \"10\", \"read_rows\": \"1\"}"))
.build());

ClickHouseHttpClient httpClient = new ClickHouseHttpClient();
Map<ClickHouseOption, Serializable> options = new HashMap<>();
options.put(ClickHouseHttpOption.AHC_RETRY_ON_FAILURE, true);
ClickHouseConfig config = new ClickHouseConfig(options);
httpClient.init(config);
ClickHouseRequest request = httpClient.read("http://localhost:9090/").query("SELECT 1");

ClickHouseResponse response = null;
try {
response = httpClient.executeAndWait(request);
} catch (ClickHouseException e) {
Assert.fail("Should not throw exception", e);
}
Assert.assertEquals(response.getSummary().getReadBytes(), 10);
Assert.assertEquals(response.getSummary().getReadRows(), 1);
} finally {
faultyServer.stop();
}
}

@DataProvider(name = "retryOnFailureProvider")
private static StubMapping[] retryOnFailureProvider() {
return new StubMapping[] {
WireMock.post(WireMock.anyUrl())
.withRequestBody(WireMock.equalTo("SELECT 1"))
.inScenario("Retry")
.whenScenarioStateIs(Scenario.STARTED)
.willReturn(WireMock.aResponse().withFault(Fault.EMPTY_RESPONSE))
.willSetStateTo("Failed")
.build()
,WireMock.post(WireMock.anyUrl())
.withRequestBody(WireMock.equalTo("SELECT 1"))
.inScenario("Retry")
.whenScenarioStateIs(Scenario.STARTED)
.willReturn(WireMock.aResponse().withStatus(HttpStatus.SC_SERVICE_UNAVAILABLE))
.willSetStateTo("Failed")
.build()
};
}
}
Loading