Skip to content

Commit

Permalink
Create new handlers for every new request in GoogleCloudStorageService (
Browse files Browse the repository at this point in the history 
#27339)

This commit changes the DefaultHttpRequestInitializer in order to make
it create new HttpIOExceptionHandler and HttpUnsuccessfulResponseHandler
for every new HTTP request instead of reusing the same two handlers for
all requests.

Closes #27092
  • Loading branch information
@tlrx
tlrx authored Nov 14, 2017
1 parent 1caa5c8 commit dd51c23
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 24 deletions.
23 changes: 5 additions & 18 deletions ...itory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import com.google.api.client.googleapis.javanet.GoogleNetHttpTransport;
import com.google.api.client.http.HttpBackOffIOExceptionHandler;
import com.google.api.client.http.HttpBackOffUnsuccessfulResponseHandler;
import com.google.api.client.http.HttpIOExceptionHandler;
import com.google.api.client.http.HttpRequest;
import com.google.api.client.http.HttpRequestInitializer;
import com.google.api.client.http.HttpUnsuccessfulResponseHandler;
Expand All @@ -34,9 +33,7 @@
import com.google.api.services.storage.StorageScopes;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.common.component.AbstractComponent;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.settings.SecureSetting;
import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
Expand All @@ -46,8 +43,6 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.UncheckedIOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
Expand Down Expand Up @@ -76,16 +71,11 @@ Storage createClient(String clientName, String application,
*/
class InternalGoogleCloudStorageService extends AbstractComponent implements GoogleCloudStorageService {

private static final String DEFAULT = "_default_";

private final Environment environment;

/** Credentials identified by client name. */
private final Map<String, GoogleCredential> credentials;

InternalGoogleCloudStorageService(Environment environment, Map<String, GoogleCredential> credentials) {
super(environment.settings());
this.environment = environment;
this.credentials = credentials;
}

Expand Down Expand Up @@ -131,15 +121,11 @@ class DefaultHttpRequestInitializer implements HttpRequestInitializer {
private final TimeValue connectTimeout;
private final TimeValue readTimeout;
private final GoogleCredential credential;
private final HttpUnsuccessfulResponseHandler handler;
private final HttpIOExceptionHandler ioHandler;

DefaultHttpRequestInitializer(GoogleCredential credential, TimeValue connectTimeout, TimeValue readTimeout) {
this.credential = credential;
this.connectTimeout = connectTimeout;
this.readTimeout = readTimeout;
this.handler = new HttpBackOffUnsuccessfulResponseHandler(newBackOff());
this.ioHandler = new HttpBackOffIOExceptionHandler(newBackOff());
}

@Override
Expand All @@ -151,13 +137,14 @@ public void initialize(HttpRequest request) throws IOException {
request.setReadTimeout((int) readTimeout.millis());
}

request.setIOExceptionHandler(ioHandler);
request.setIOExceptionHandler(new HttpBackOffIOExceptionHandler(newBackOff()));
request.setInterceptor(credential);

final HttpUnsuccessfulResponseHandler handler = new HttpBackOffUnsuccessfulResponseHandler(newBackOff());
request.setUnsuccessfulResponseHandler((req, resp, supportsRetry) -> {
// Let the credential handle the response. If it failed, we rely on our backoff handler
return credential.handleResponse(req, resp, supportsRetry) || handler.handleResponse(req, resp, supportsRetry);
}
// Let the credential handle the response. If it failed, we rely on our backoff handler
return credential.handleResponse(req, resp, supportsRetry) || handler.handleResponse(req, resp, supportsRetry);
}
);
}

Expand Down
73 changes: 67 additions & 6 deletions ...-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageServiceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,36 @@

package org.elasticsearch.repositories.gcs;

import java.io.IOException;
import java.io.InputStream;
import java.util.Collections;
import java.util.Map;

import com.google.api.client.googleapis.auth.oauth2.GoogleCredential;
import com.google.api.client.http.GenericUrl;
import com.google.api.client.http.HttpIOExceptionHandler;
import com.google.api.client.http.HttpRequest;
import com.google.api.client.http.HttpRequestFactory;
import com.google.api.client.http.HttpRequestInitializer;
import com.google.api.client.http.HttpResponse;
import com.google.api.client.http.HttpUnsuccessfulResponseHandler;
import com.google.api.client.testing.http.MockHttpTransport;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.env.Environment;
import org.elasticsearch.env.TestEnvironment;
import org.elasticsearch.repositories.gcs.GoogleCloudStorageService.InternalGoogleCloudStorageService;
import org.elasticsearch.test.ESTestCase;

import java.io.IOException;
import java.io.InputStream;
import java.util.Collections;
import java.util.Map;

import static java.util.Collections.emptyMap;
import static java.util.Collections.singletonMap;
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyBoolean;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

public class GoogleCloudStorageServiceTests extends ESTestCase {

private InputStream getDummyCredentialStream() throws IOException {
Expand All @@ -47,13 +65,56 @@ GoogleCredential getDefaultCredential() throws IOException {
}
};
assertSame(cred, service.getCredential("default"));

service.new DefaultHttpRequestInitializer(cred, null, null);
}

public void testClientCredential() throws Exception {
GoogleCredential cred = GoogleCredential.fromStream(getDummyCredentialStream());
Map<String, GoogleCredential> credentials = Collections.singletonMap("clientname", cred);
Map<String, GoogleCredential> credentials = singletonMap("clientname", cred);
Environment env = TestEnvironment.newEnvironment(Settings.builder().put("path.home", createTempDir()).build());
InternalGoogleCloudStorageService service = new InternalGoogleCloudStorageService(env, credentials);
assertSame(cred, service.getCredential("clientname"));
}

/**
* Test that the {@link InternalGoogleCloudStorageService.DefaultHttpRequestInitializer} attaches new instances
* of {@link HttpIOExceptionHandler} and {@link HttpUnsuccessfulResponseHandler} for every HTTP requests.
*/
public void testDefaultHttpRequestInitializer() throws IOException {
final Environment environment = mock(Environment.class);
when(environment.settings()).thenReturn(Settings.EMPTY);

final GoogleCredential credential = mock(GoogleCredential.class);
when(credential.handleResponse(any(HttpRequest.class), any(HttpResponse.class), anyBoolean())).thenReturn(false);

final TimeValue readTimeout = TimeValue.timeValueSeconds(randomIntBetween(1, 120));
final TimeValue connectTimeout = TimeValue.timeValueSeconds(randomIntBetween(1, 120));

final InternalGoogleCloudStorageService service = new InternalGoogleCloudStorageService(environment, emptyMap());
final HttpRequestInitializer initializer = service.new DefaultHttpRequestInitializer(credential, connectTimeout, readTimeout);
final HttpRequestFactory requestFactory = new MockHttpTransport().createRequestFactory(initializer);

final HttpRequest request1 = requestFactory.buildGetRequest(new GenericUrl());
assertEquals((int) connectTimeout.millis(), request1.getConnectTimeout());
assertEquals((int) readTimeout.millis(), request1.getReadTimeout());
assertSame(credential, request1.getInterceptor());
assertNotNull(request1.getIOExceptionHandler());
assertNotNull(request1.getUnsuccessfulResponseHandler());

final HttpRequest request2 = requestFactory.buildGetRequest(new GenericUrl());
assertEquals((int) connectTimeout.millis(), request2.getConnectTimeout());
assertEquals((int) readTimeout.millis(), request2.getReadTimeout());
assertSame(request1.getInterceptor(), request2.getInterceptor());
assertNotNull(request2.getIOExceptionHandler());
assertNotSame(request1.getIOExceptionHandler(), request2.getIOExceptionHandler());
assertNotNull(request2.getUnsuccessfulResponseHandler());
assertNotSame(request1.getUnsuccessfulResponseHandler(), request2.getUnsuccessfulResponseHandler());

request1.getUnsuccessfulResponseHandler().handleResponse(null, null, false);
verify(credential, times(1)).handleResponse(any(HttpRequest.class), any(HttpResponse.class), anyBoolean());

request2.getUnsuccessfulResponseHandler().handleResponse(null, null, false);
verify(credential, times(2)).handleResponse(any(HttpRequest.class), any(HttpResponse.class), anyBoolean());
}
}

0 comments on commit dd51c23

Please sign in to comment.