From d2f82f8dd624d15c7fd5b26ca710d6751bf78291 Mon Sep 17 00:00:00 2001 From: imply-cheddar <86940447+imply-cheddar@users.noreply.github.com> Date: Thu, 13 Apr 2023 15:10:50 +0900 Subject: [PATCH] Make GCP initialization truly lazy (#14077) The GCP initialization pulls credentials for talking to GCP. We want that to only happen when fully required and thus want the GCP-related objects lazily instantiated. --- .../druid/common/gcp/GcpMockModule.java | 12 ++-- .../druid/common/gcp/GcpModuleTest.java | 14 ++++- .../google/GoogleStorageDruidModule.java | 36 ++++++------ .../google/GoogleStorageDruidModuleTest.java | 56 +++++++++++++++++-- 4 files changed, 92 insertions(+), 26 deletions(-) diff --git a/cloud/gcp-common/src/test/java/org/apache/druid/common/gcp/GcpMockModule.java b/cloud/gcp-common/src/test/java/org/apache/druid/common/gcp/GcpMockModule.java index b8d6048b4ae..4fa59c80a2b 100644 --- a/cloud/gcp-common/src/test/java/org/apache/druid/common/gcp/GcpMockModule.java +++ b/cloud/gcp-common/src/test/java/org/apache/druid/common/gcp/GcpMockModule.java @@ -19,7 +19,6 @@ package org.apache.druid.common.gcp; -import com.google.api.client.googleapis.testing.auth.oauth2.MockGoogleCredential; import com.google.api.client.http.HttpRequestInitializer; import com.google.api.client.http.HttpTransport; import com.google.api.client.json.JsonFactory; @@ -30,7 +29,7 @@ import com.google.inject.Provides; import org.apache.druid.guice.LazySingleton; import org.apache.druid.initialization.DruidModule; -public class GcpMockModule implements DruidModule +public abstract class GcpMockModule implements DruidModule { @Override public void configure(Binder binder) @@ -39,14 +38,19 @@ public class GcpMockModule implements DruidModule @Provides @LazySingleton - public HttpRequestInitializer mockRequestInitializer( + public HttpRequestInitializer provideRequestInitializer( HttpTransport transport, JsonFactory factory ) { - return new MockGoogleCredential.Builder().setTransport(transport).setJsonFactory(factory).build(); + return mockRequestInitializer(transport, factory); } + public abstract HttpRequestInitializer mockRequestInitializer( + HttpTransport transport, + JsonFactory factory + ); + @Provides @LazySingleton public HttpTransport buildMockTransport() diff --git a/cloud/gcp-common/src/test/java/org/apache/druid/common/gcp/GcpModuleTest.java b/cloud/gcp-common/src/test/java/org/apache/druid/common/gcp/GcpModuleTest.java index 4790a914b72..3813ef55b27 100644 --- a/cloud/gcp-common/src/test/java/org/apache/druid/common/gcp/GcpModuleTest.java +++ b/cloud/gcp-common/src/test/java/org/apache/druid/common/gcp/GcpModuleTest.java @@ -22,12 +22,13 @@ package org.apache.druid.common.gcp; import com.google.api.client.googleapis.testing.auth.oauth2.MockGoogleCredential; import com.google.api.client.http.HttpRequestInitializer; import com.google.api.client.http.HttpTransport; +import com.google.api.client.json.JsonFactory; import com.google.api.client.testing.http.MockHttpTransport; import com.google.inject.Binder; import com.google.inject.Guice; import com.google.inject.Injector; -import com.google.inject.Scopes; import com.google.inject.util.Modules; +import org.apache.druid.guice.DruidScopes; import org.apache.druid.guice.LazySingleton; import org.junit.Assert; import org.junit.Test; @@ -42,7 +43,16 @@ public class GcpModuleTest @Override public void configure(Binder binder) { - binder.bindScope(LazySingleton.class, Scopes.SINGLETON); + binder.bindScope(LazySingleton.class, DruidScopes.SINGLETON); + } + + @Override + public HttpRequestInitializer mockRequestInitializer( + HttpTransport transport, + JsonFactory factory + ) + { + return new MockGoogleCredential.Builder().setTransport(transport).setJsonFactory(factory).build(); } })); Assert.assertTrue(injector.getInstance(HttpRequestInitializer.class) instanceof MockGoogleCredential); diff --git a/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleStorageDruidModule.java b/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleStorageDruidModule.java index 8aa77b0298f..cb90cb51fb8 100644 --- a/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleStorageDruidModule.java +++ b/extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleStorageDruidModule.java @@ -27,9 +27,9 @@ import com.google.api.client.http.HttpRequestInitializer; import com.google.api.client.http.HttpTransport; import com.google.api.client.json.JsonFactory; import com.google.api.services.storage.Storage; -import com.google.common.base.Suppliers; import com.google.common.collect.ImmutableList; import com.google.inject.Binder; +import com.google.inject.Provider; import com.google.inject.Provides; import com.google.inject.multibindings.MapBinder; import org.apache.druid.data.SearchableVersionedDataFinder; @@ -97,9 +97,23 @@ public class GoogleStorageDruidModule implements DruidModule JsonConfigProvider.bind(binder, "druid.indexer.logs", GoogleTaskLogsConfig.class); binder.bind(GoogleTaskLogs.class).in(LazySingleton.class); MapBinder.newMapBinder(binder, String.class, SearchableVersionedDataFinder.class) - .addBinding(SCHEME_GS) - .to(GoogleTimestampVersionedDataFinder.class) - .in(LazySingleton.class); + .addBinding(SCHEME_GS) + .to(GoogleTimestampVersionedDataFinder.class) + .in(LazySingleton.class); + } + + @Provides + @LazySingleton + public Storage getGcpStorage( + HttpTransport httpTransport, + JsonFactory jsonFactory, + HttpRequestInitializer requestInitializer + ) + { + return new Storage + .Builder(httpTransport, jsonFactory, requestInitializer) + .setApplicationName(APPLICATION_NAME) + .build(); } /** @@ -109,20 +123,10 @@ public class GoogleStorageDruidModule implements DruidModule @Provides @LazySingleton public GoogleStorage getGoogleStorage( - HttpTransport httpTransport, - JsonFactory jsonFactory, - HttpRequestInitializer requestInitializer + Provider baseStorageProvider ) { LOG.info("Building Cloud Storage Client..."); - - return new GoogleStorage( - Suppliers.memoize( - () -> new Storage - .Builder(httpTransport, jsonFactory, requestInitializer) - .setApplicationName(APPLICATION_NAME) - .build() - ) - ); + return new GoogleStorage(baseStorageProvider::get); } } diff --git a/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleStorageDruidModuleTest.java b/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleStorageDruidModuleTest.java index e8000f5a4b8..9ca384cf468 100644 --- a/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleStorageDruidModuleTest.java +++ b/extensions-core/google-extensions/src/test/java/org/apache/druid/storage/google/GoogleStorageDruidModuleTest.java @@ -19,6 +19,11 @@ package org.apache.druid.storage.google; +import com.google.api.client.googleapis.testing.auth.oauth2.MockGoogleCredential; +import com.google.api.client.http.HttpRequestInitializer; +import com.google.api.client.http.HttpTransport; +import com.google.api.client.json.JsonFactory; +import com.google.api.services.storage.Storage; import com.google.common.collect.ImmutableList; import com.google.inject.Injector; import org.apache.druid.common.gcp.GcpMockModule; @@ -32,22 +37,65 @@ public class GoogleStorageDruidModuleTest @Test public void testSegmentKillerBoundedSingleton() { - Injector injector = createInjector(); + // This test is primarily validating 2 things + // 1. That the Google credentials are loaded lazily, they are loaded as part of instantiation of the + // HttpRquestInitializer, the test throws an exception from that method, meaning that if they are not loaded + // lazily, the exception should end up thrown. + // 2. That the same object is returned. + Injector injector = GuiceInjectors.makeStartupInjectorWithModules( + ImmutableList.of( + new GcpMockModule() + { + + @Override + public HttpRequestInitializer mockRequestInitializer( + HttpTransport transport, + JsonFactory factory + ) + { + return new MockGoogleCredential.Builder().setTransport(transport).setJsonFactory(factory).build(); + } + }, + new GoogleStorageDruidModule() + ) + ); OmniDataSegmentKiller killer = injector.getInstance(OmniDataSegmentKiller.class); Assert.assertTrue(killer.getKillers().containsKey(GoogleStorageDruidModule.SCHEME)); Assert.assertSame( killer.getKillers().get(GoogleStorageDruidModule.SCHEME).get(), killer.getKillers().get(GoogleStorageDruidModule.SCHEME).get() ); + + final Storage storage = injector.getInstance(Storage.class); + Assert.assertSame(storage, injector.getInstance(Storage.class)); } - private static Injector createInjector() + @Test + public void testLazyInstantiation() { - return GuiceInjectors.makeStartupInjectorWithModules( + // This test is primarily validating 2 things + // 1. That the Google credentials are loaded lazily, they are loaded as part of instantiation of the + // HttpRquestInitializer, the test throws an exception from that method, meaning that if they are not loaded + // lazily, the exception should end up thrown. + // 2. That the same object is returned. + Injector injector = GuiceInjectors.makeStartupInjectorWithModules( ImmutableList.of( - new GcpMockModule(), + new GcpMockModule() + { + + @Override + public HttpRequestInitializer mockRequestInitializer( + HttpTransport transport, + JsonFactory factory + ) + { + throw new UnsupportedOperationException("should not be called, because this should be lazy"); + } + }, new GoogleStorageDruidModule() ) ); + final GoogleStorage instance = injector.getInstance(GoogleStorage.class); + Assert.assertSame(instance, injector.getInstance(GoogleStorage.class)); } }