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.

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

Add basic native-image configuration #1318

Closed
wants to merge 1 commit into from

Conversation

christophstrobl
Copy link
Contributor

@christophstrobl christophstrobl commented Jun 23, 2020

A starting point for further improvements.

Add required proxy-config and reflect-config including netty setup allowing unsafe access to certain fields.
Also register DefaultCommandLatencyCollector for build time initialization.

Relates to: #1316

@codecov
Copy link

codecov bot commented Jun 23, 2020

Codecov Report

Merging #1318 into main will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1318      +/-   ##
============================================
+ Coverage     80.22%   80.24%   +0.01%     
- Complexity     6081     6085       +4     
============================================
  Files           429      429              
  Lines         20018    20018              
  Branches       2265     2265              
============================================
+ Hits          16060    16064       +4     
+ Misses         2912     2909       -3     
+ Partials       1046     1045       -1     
Impacted Files Coverage Δ Complexity Δ
...a/io/lettuce/core/protocol/ConnectionWatchdog.java 79.08% <0.00%> (-0.66%) 39.00% <0.00%> (ø%)
.../io/lettuce/core/dynamic/ReactiveTypeAdapters.java 88.26% <0.00%> (+0.43%) 1.00% <0.00%> (ø%)
...java/io/lettuce/core/protocol/DefaultEndpoint.java 70.02% <0.00%> (+0.46%) 97.00% <0.00%> (ø%)
...ttuce/core/support/ClientResourcesFactoryBean.java 60.00% <0.00%> (+13.33%) 5.00% <0.00%> (+1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a1a76b...613d948. Read the comment docs.

@ilopmar
Copy link

ilopmar commented Jun 23, 2020

Hi @christophstrobl, I've tried with the configuration proposed in this PR with the simple test application I created in #1316 and it doesn't work.

Could you please take a look at the reflection and proxy configuration I used here? https://github.com/ilopmar/redis-lettuce-graalvm/tree/master/src/main/resources/META-INF/native-image/example/redis-lettuce

Copy link
Collaborator

@mp911de mp911de left a comment

Choose a reason for hiding this comment

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

I went through our code and added a few more items that should go into the Graal native image config.

I can take this PR from here.

@@ -0,0 +1,233 @@
[
{
"name":"io.netty.util.internal.shaded.org.jctools.queues.BaseMpscLinkedArrayQueueColdProducerFields",
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are netty-related bits. We should remove these and leave configuration up to netty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 feel free to open an issue there

@@ -0,0 +1,3 @@
[
["io.lettuce.core.api.sync.RedisCommands","io.lettuce.core.cluster.api.sync.RedisClusterCommands"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need also the following types (going through all places where we create proxies):

  • AsyncNodeSelection
  • HasTargetConnection
  • NodeSelection
  • NodeSelectionAsyncCommands
  • NodeSelectionCommands
  • NodeSelectionPubSubAsyncCommands
  • NodeSelectionPubSubCommands
  • NodeSelectionPubSubReactiveCommands
  • PubSubAsyncNodeSelection
  • PubSubNodeSelection
  • PubSubReactiveNodeSelection
  • RedisAdvancedClusterCommands
  • RedisClusterAsyncCommands
  • RedisClusterCommands
  • RedisClusterPubSubCommands
  • RedisCommands
  • RedisPubSubAsyncCommands
  • RedisPubSubCommands
  • RedisSentinelCommands

"allDeclaredConstructors":true
},
{
"name":"io.netty.buffer.AbstractByteBufAllocator",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Netty types should be managed by netty.

},

{
"name":"io.lettuce.core.protocol.CommandHandler",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We also need:

  • PubSubCommandHandler
  • SslChannelInitializer
  • SslRedisHandshakeHandler (6.0)
  • RedisHandshakeHandler (6.0)

Not sure about CommandEncoder

@@ -0,0 +1,3 @@
[
["io.lettuce.core.api.sync.RedisCommands","io.lettuce.core.cluster.api.sync.RedisClusterCommands"]
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

For dynamic command proxies we additionally need:

  ["java.lang.reflect.ParameterizedType","io.lettuce.core.dynamic.support.TypeWrapper$SerializableTypeProxy","java.io.Serializable", "java.lang.reflect.TypeVariable"],

"allPublicMethods": true,
"allDeclaredConstructors":true
}
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Additional types (for latency tracking, since we do class presence checks)

  • org.HdrHistogram.Histogram
  • org.LatencyUtils.PauseDetector
  • rx.Completable
  • reactor.core.publisher.Mono
  • io.reactivex.Flowable
  • io.reactivex.rxjava3.core.Flowable

@mp911de mp911de changed the title #1316 - Add basic native-image configuration. Add basic native-image configuration Jun 23, 2020
@ilopmar
Copy link

ilopmar commented Jun 23, 2020

Thank you @mp911de. Another thing I haven't managed to get it working is the HdrHistogram and LatencyUtils dependencies. Without them my simple app fails.
I'm happy to test this PR once it's ready, so feel free to ping me :-)

@mp911de mp911de added this to the 5.3.2 milestone Jun 23, 2020
@mp911de mp911de added the type: feature A new feature label Jun 23, 2020
@christophstrobl
Copy link
Contributor Author

@ilopmar, as mentioned, it's a starting point (which worked for what I did). @mp911de already identified additional spots that require configuration.

@ilopmar
Copy link

ilopmar commented Jun 23, 2020

@christophstrobl sorry, I missread that. Understood :-)

mp911de pushed a commit that referenced this pull request Jun 23, 2020
mp911de added a commit that referenced this pull request Jun 23, 2020
Extend reflection and proxy configuration. Add documentation

Original pull request: #1318.
mp911de pushed a commit that referenced this pull request Jun 23, 2020
mp911de added a commit that referenced this pull request Jun 23, 2020
Extend reflection and proxy configuration. Add documentation

Original pull request: #1318.
@mp911de
Copy link
Collaborator

mp911de commented Jun 23, 2020

Thank you for your contribution. That's merged, polished, and backported now.

@mp911de mp911de closed this Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants