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

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

Merged
merged 7 commits into from
Mar 30, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6bc88ce
fix(java): add configurations for Storage classes and tests
mpeddada1 Mar 22, 2022
dbb033c
move resource config to test folder
mpeddada1 Mar 23, 2022
fbca4fc
fix directory path for test configurations
mpeddada1 Mar 23, 2022
5553f5d
move all configurations to test folder
mpeddada1 Mar 24, 2022
ec051f6
organize configurations; document class initialization
mpeddada1 Mar 28, 2022
e8a3b62
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] Mar 28, 2022
e8e817a
Merge branch 'main' into add-native-config
mpeddada1 Mar 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...orage/src/main/resources/META-INF/native-image/com/google/cloud/storage/proxy-config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[
{
"interfaces":["com.google.cloud.storage.spi.v1.StorageRpc"]
}
]
BenWhitehead marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 4 additions & 0 deletions ...age/src/main/resources/META-INF/native-image/com/google/cloud/storage/reflect-config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
[{
"name":"com.google.cloud.storage.BlobInfo$ImmutableEmptyMap",
"methods":[{"name":"<init>","parameterTypes":[] }]}
]
32 changes: 32 additions & 0 deletions ...src/test/resources/META-INF/native-image/com/google/cloud/storage/native-image.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# The JUnitFeature which is brought in by Graal 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. Initializing these classes explicitly at build time results in a
# successful build.
Args = \
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@mpeddada1 mpeddada1 Mar 25, 2022 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Member

@suztomo suztomo Mar 25, 2022 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

--initialize-at-build-time=com.google.cloud.conformance.storage.v1,\
com.google.protobuf,\
com.google.auth.oauth2,\
com.google.cloud.storage.conformance.retry,\
com.google.common.base.Charsets,\
com.google.gson.stream.JsonReader,\
com.google.api.client.util,\
com.google.api.client.http.javanet.NetHttpTransport,\
com.google.api.client.http.HttpTransport,\
com.google.api.client.json,\
com.google.common.io.BaseEncoding,\
com.google.common.math.IntMath$1,\
com.google.common.collect.Platform,\
com.google.gson.Gson,\
com.google.common.truth,\
com.google.common.collect,\
com.google.gson.internal.reflect,\
com.google.gson.internal.bind,\
com.google.gson.internal,\
com.google.gson.internal.sql.SqlTypesSupport,\
com.google.gson.FieldNamingPolicy$3,\
com.google.gson.LongSerializationPolicy$2



Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

22 changes: 22 additions & 0 deletions ...age/src/test/resources/META-INF/native-image/com/google/cloud/storage/reflect-config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
[
{
"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":[] }]}
,
{
"name":"com.google.cloud.storage.conformance.retry.TestBench$RetryTestResource",
"allDeclaredFields":true,
"methods":[{"name":"<init>","parameterTypes":[] }]}
]
7 changes: 7 additions & 0 deletions ...ge/src/test/resources/META-INF/native-image/com/google/cloud/storage/resource-config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"resources":{
"includes":[
{"pattern": ".*.txt"}
mpeddada1 marked this conversation as resolved.
Show resolved Hide resolved
]
}
}