fix(java): add configurations for Storage tests #1305

mpeddada1 · 2022-03-23T15:52:16Z

This PR adds a few reflection configurations for native image support in Storage.

prox-config.json:

JUnit Vintage:ITBlobWriteChannelTest:testJsonEOF_0B
    MethodSource [className = 'com.google.cloud.storage.it.ITBlobWriteChannelTest', methodName = 'testJsonEOF_0B', methodParameterTypes = '']
    => com.oracle.svm.core.jdk.UnsupportedFeatureError: Proxy class defined by interfaces [interface com.google.cloud.storage.spi.v1.StorageRpc] not found. Generating proxy classes at runtime is not supported. Proxy classes need to be defined at image build time by specifying the list of interfaces that they implement. To define proxy classes use -H:DynamicProxyConfigurationFiles=<comma-separated-config-files> and -H:DynamicProxyConfigurationResources=<comma-separated-config-resources> options.
       com.oracle.svm.core.util.VMError.unsupportedFeature(VMError.java:87)
       com.oracle.svm.reflect.proxy.DynamicProxySupport.getProxyClass(DynamicProxySupport.java:146)
       java.lang.reflect.Proxy.getProxyConstructor(Proxy.java:66)
       java.lang.reflect.Proxy.newProxyInstance(Proxy.java:1006)
       com.google.common.reflect.Reflection.newProxy(Reflection.java:86)
       com.google.cloud.storage.it.ITBlobWriteChannelTest.lambda$doJsonUnexpectedEOFTest$1(ITBlobWriteChannelTest.java:140)
       com.google.cloud.ServiceOptions.getRpc(ServiceOptions.java:560)
       com.google.cloud.storage.StorageOptions.getStorageRpcV1(StorageOptions.java:149)
       com.google.cloud.storage.StorageImpl.<init>(StorageImpl.java:115)
       com.google.cloud.storage.StorageOptions$DefaultStorageFactory.create(StorageOptions.java:50)

The reflect-config.json file registers some Java classes for reflection:

 JUnit Vintage:ITStorageTest:testUpdateBlobReplaceMetadata
    MethodSource [className = 'com.google.cloud.storage.it.ITStorageTest', methodName = 'testUpdateBlobReplaceMetadata', methodParameterTypes = '']
    => java.lang.IllegalArgumentException: unable to create new instance of class com.google.cloud.storage.BlobInfo$ImmutableEmptyMap because it has no accessible default constructor
       com.google.api.client.util.Types.handleExceptionForNewInstance(Types.java:162)
       com.google.api.client.util.Types.newInstance(Types.java:117)
       com.google.api.client.util.Data.createNullInstance(Data.java:166)
       com.google.api.client.util.Data.nullOf(Data.java:134)
       com.google.cloud.storage.BlobInfo$BuilderImpl.setMetadata(BlobInfo.java:585)
       [...]
     Caused by: java.lang.InstantiationException: Type `com.google.cloud.storage.BlobInfo.ImmutableEmptyMap` can not be instantiated reflectively as it does not have a no-parameter constructor or the no-parameter constructor has not been added explicitly to the native image.
       java.lang.Class.newInstance(DynamicHub.java:909)
       com.google.api.client.util.Types.newInstance(Types.java:113)
       [...]

The resource-config.json file makes certain test resource accessible to the native image builder. Specifically, testNamesWhichCanFail.txt. It is currently located under src/test as it applies to only tests.

The native-image.properties file is located under src/test as it only applies to the tests :

This file handles class initialization during native image generation. Classes can be initialized at either run-time or image build time and class initialization can result in recursive initialization of other classes. The tests in the Storage repo use Parameterized which is explicitly initialized at image build time by the configurations provided by GraalVM. This leads to a bunch of other unexpected classes being initialized at build time as well and leads to the following error:

Error: Classes that should be initialized at run time got initialized during image building:
 com.google.cloud.conformance.storage.v1.RetryTests was unintentionally initialized at build time. To see why com.google.cloud.conformance.storage.v1.RetryTests got initialized use --trace-class-initialization=com.google.cloud.conformance.storage.v1.RetryTests
com.google.cloud.conformance.storage.v1.InstructionList was unintentionally initialized at build time. To see why com.google.cloud.conformance.storage.v1.InstructionList got initialized use --trace-class-initialization=com.google.cloud.conformance.storage.v1.InstructionList

And initializing those same classes at run-time results in image generation failing with inconclusive results:

Error: Classes that should be initialized at run time got initialized during image building:
 com.google.cloud.storage.conformance.retry.ITRetryConformanceTest$RetryTestCaseResolver the class was requested to be initialized at run time

Initializing these classes explicitly at build-time results in a successful build.

...orage/src/test/resources/META-INF/native-image/com/google/cloud/storage/resource-config.json

BenWhitehead · 2022-03-23T21:16:17Z

...torage/src/main/resources/META-INF/native-image/com/google/cloud/storage/reflect-config.json

+  {
+    "name":"org.apache.commons.logging.LogFactory",
+    "allDeclaredFields":true,
+    "allDeclaredMethods":true,
+    "allDeclaredConstructors": true
+  },
+  {
+    "name":"org.apache.commons.logging.impl.Jdk14Logger",
+    "methods":[{"name":"<init>","parameterTypes":["java.lang.String"] }]}
+,
+  {
+    "name":"org.apache.commons.logging.impl.LogFactoryImpl",
+    "allDeclaredFields":true,
+    "allDeclaredMethods":true,
+    "methods":[{"name":"<init>","parameterTypes":[] }]}
+,


We're not supposed to have any dependency on apache logging. Do you know how I might find the reference path for these classes to try and eliminate them?

I looked into this a bit by removing the LogFactoryImpl configuration and running mvn clean install -DskipTests and mvn test -Pnative. This results in the following stacktrace:

JUnit Vintage:ITStorageTest:testWriteChannelWithConnectionPool MethodSource [className = 'com.google.cloud.storage.it.ITStorageTest', methodName = 'testWriteChannelWithConnectionPool', methodParameterTypes = ''] => java.lang.ExceptionInInitializerError org.apache.http.conn.ssl.SSLConnectionSocketFactory.<clinit>(SSLConnectionSocketFactory.java:151) org.apache.http.impl.conn.PoolingHttpClientConnectionManager.getDefaultRegistry(PoolingHttpClientConnectionManager.java:116) org.apache.http.impl.conn.PoolingHttpClientConnectionManager.<init>(PoolingHttpClientConnectionManager.java:123) com.google.cloud.storage.it.ITStorageTest$CustomHttpTransportFactory.create(ITStorageTest.java:282) com.google.cloud.storage.spi.v1.HttpStorageRpc.<init>(HttpStorageRpc.java:110) [...] Caused by: org.apache.commons.logging.LogConfigurationException: java.lang.ClassNotFoundException: org.apache.commons.logging.impl.LogFactoryImpl (Caused by java.lang.ClassNotFoundException: org.apache.commons.logging.impl.LogFactoryImpl) org.apache.commons.logging.LogFactory.createFactory(LogFactory.java:1158) org.apache.commons.logging.LogFactory$2.run(LogFactory.java:960) java.security.AccessController.doPrivileged(AccessController.java:87) org.apache.commons.logging.LogFactory.newFactory(LogFactory.java:957) org.apache.commons.logging.LogFactory.getFactory(LogFactory.java:624) [...] Caused by: java.lang.ClassNotFoundException: org.apache.commons.logging.impl.LogFactoryImpl jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:53) jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178) java.lang.ClassLoader.loadClass(ClassLoader.java:139) org.apache.commons.logging.LogFactory.createFactory(LogFactory.java:1020) [...]

It looks the usage of LogFactory is coming from PoolingHttpClientConnectionManager which is referenced in ITStorageTest.

Ah okay, so that's testing for an optional feature. Thanks for trying it out

...-storage/src/main/resources/META-INF/native-image/com/google/cloud/storage/proxy-config.json

BenWhitehead · 2022-03-23T21:21:15Z

...ge/src/test/resources/META-INF/native-image/com/google/cloud/storage/native-image.properties

+  com.google.gson.LongSerializationPolicy$2
+
+
+


Does this mean that com.google.cloud.storage.conformance.retry.ITRetryConformanceTest#testCases is being invoked during the building of the native image?

We're using @RunWith(ParallelParameterized.class) to resolve test cases relative to a classpath resource, and run them in parallel using a threadpool.

While standard Java initializes classes the first time they are accessed at run-time, Native image compilation tries to do some optimizations were it initializes some classes at image build time to decrease the start-up time of an application.

In our case, the JunitFeature which is brought in by the native-maven-plugin (plugin we use to run tests with native image compilation) explicitly initializes Parameterized at image build time. This causes ParallelParameterized and subsequently com.google.cloud.storage.conformance.retry.ITRetryConformanceTest and other classes ITRetryConformanceTest references to also be initialized at build time. While the class itself is initialized at build time, it still runs at run-time (after the native image has been built).

This article has been helping me understand class initialization a bit in graal.

...torage/src/main/resources/META-INF/native-image/com/google/cloud/storage/reflect-config.json

mpeddada1 · 2022-03-24T23:29:38Z

@BenWhitehead any idea why some checks are stalled at the moment?

suztomo · 2022-03-25T20:45:50Z

...ge/src/test/resources/META-INF/native-image/com/google/cloud/storage/native-image.properties

@@ -0,0 +1,26 @@
+Args = \


The file that tells build-time initialization of classes of the library shouldn't be in test. Because the file is not shipped as part of the library.

As discussed today, this file being under the test directory means the users would have to write something similar to use google-cloud-storage with native-image.

The only reason why these configurations are initialized at build-time is because com.google.cloud.storage.conformance.retry.ITRetryConformanceTest which references them is initialized at build-time (following this logic). I'm not sure if this will apply to the regular application? WDYT

That's my misunderstanding then. Sorry I missed the discussion. Would you add that comment in the file about the configuration being for the test classes?

An explanation on why so many classes (not test classes) are needed here is nice.

Thanks for the suggestion! Added a comment to summarize this.

Now the --initialize-at-build-time doesn't have google-cloud-storage's main package. Nice.

suztomo · 2022-03-26T01:47:00Z

...torage/src/test/resources/META-INF/native-image/com/google/cloud/storage/reflect-config.json

+    "methods":[{"name":"<init>","parameterTypes":[] }]}
+,
+{
+  "name":"com.google.cloud.storage.BlobInfo$ImmutableEmptyMap",


Would you add comment explaining why this configuration is not in main?

Sorry, my bad. I unintentionally placed this class config in the test directory. This configuration is used outside of the tests so needs to in the main/resources directory.

suztomo · 2022-03-26T01:47:21Z

...-storage/src/test/resources/META-INF/native-image/com/google/cloud/storage/proxy-config.json

@@ -0,0 +1,5 @@
+[
+  {
+    "interfaces":["com.google.cloud.storage.spi.v1.StorageRpc"]


Would you add comment explaining why this configuration is not in main?

Thanks for point this out! I realized that this class also needs to be in the main config directory since it is referenced outside of tests. For example:

java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java

Line 90 in 3716105

public class HttpStorageRpc implements StorageRpc {

Apologies for the confusion @BenWhitehead!

suztomo · 2022-03-26T01:51:15Z

Now the checks are running (Kokoro sample failed)

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

BenWhitehead

Verified config by running:

export JAVA_HOME=~/opt/java/11-native
mvn -Pnative clean verify

BenWhitehead · 2022-03-28T22:05:58Z

Feel free to merge when you're ready. The test failure in Samples is being dealt with separately and isn't related to any change here.

mpeddada1 · 2022-03-30T13:24:47Z

Thank you!

mpeddada1 added 2 commits March 23, 2022 09:58

fix(java): add configurations for Storage classes and tests

6bc88ce

move resource config to test folder

dbb033c

mpeddada1 requested review from a team as code owners March 23, 2022 15:52

mpeddada1 requested review from suztomo, meltsufin and Neenu1995 March 23, 2022 15:53

product-auto-label bot added the api: storage Issues related to the googleapis/java-storage API. label Mar 23, 2022

fix directory path for test configurations

fbca4fc

BenWhitehead reviewed Mar 23, 2022

View reviewed changes

move all configurations to test folder

5553f5d

mpeddada1 changed the title ~~fix(java): add configurations for Storage classes and tests~~ fix(java): add configurations for Storage tests Mar 24, 2022

suztomo closed this Mar 25, 2022

suztomo reopened this Mar 25, 2022

suztomo reviewed Mar 25, 2022

View reviewed changes

suztomo reviewed Mar 26, 2022

View reviewed changes

mpeddada1 and others added 2 commits March 28, 2022 10:52

organize configurations; document class initialization

ec051f6

🦉 Updates from OwlBot post-processor

e8a3b62

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

BenWhitehead approved these changes Mar 28, 2022

View reviewed changes

suztomo approved these changes Mar 29, 2022

View reviewed changes

Merge branch 'main' into add-native-config

e8e817a

mpeddada1 merged commit 2bacf92 into main Mar 30, 2022

mpeddada1 deleted the add-native-config branch March 30, 2022 13:25

release-please bot mentioned this pull request Mar 30, 2022

chore(main): release 2.6.0 #1322

Merged

release-please bot mentioned this pull request Aug 10, 2022

chore(main): release 2.11.4 #1564

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(java): add configurations for Storage tests #1305

fix(java): add configurations for Storage tests #1305

mpeddada1 commented Mar 23, 2022

BenWhitehead Mar 23, 2022

mpeddada1 Mar 24, 2022

BenWhitehead Mar 24, 2022

BenWhitehead Mar 23, 2022

mpeddada1 Mar 24, 2022

mpeddada1 commented Mar 24, 2022

suztomo Mar 25, 2022

mpeddada1 Mar 25, 2022 •

edited

suztomo Mar 25, 2022 •

edited

mpeddada1 Mar 28, 2022

suztomo Mar 29, 2022

suztomo Mar 26, 2022

mpeddada1 Mar 28, 2022

suztomo Mar 26, 2022

mpeddada1 Mar 28, 2022

suztomo commented Mar 26, 2022

BenWhitehead left a comment

BenWhitehead commented Mar 28, 2022

mpeddada1 commented Mar 30, 2022

fix(java): add configurations for Storage tests #1305

fix(java): add configurations for Storage tests #1305

Conversation

mpeddada1 commented Mar 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpeddada1 commented Mar 24, 2022

Choose a reason for hiding this comment

mpeddada1 Mar 25, 2022 • edited

Choose a reason for hiding this comment

suztomo Mar 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suztomo commented Mar 26, 2022

BenWhitehead left a comment

Choose a reason for hiding this comment

BenWhitehead commented Mar 28, 2022

mpeddada1 commented Mar 30, 2022

mpeddada1 Mar 25, 2022 •

edited

suztomo Mar 25, 2022 •

edited