ISPN-8657 Netty upgrade to 4.1.21 #5676

slaskawi · 2018-01-15T12:53:38Z

https://issues.jboss.org/browse/ISPN-8657

rvansa · 2018-01-15T13:02:44Z

server/integration/feature-pack/pom.xml

@@ -528,6 +528,11 @@
            <artifactId>netty-transport-native-epoll</artifactId>
        </dependency>

+        <dependency>
+            <groupId>io.netty</groupId>
+            <artifactId>netty-transport-native-unix-common</artifactId>


why do we need this?

might make more sense to pull in the bom in dependencyManagement?

<dependency> <groupId>io.netty</groupId> <artifactId>netty-bom</artifactId> <version>${version.netty}</version> <type>pom</type> <scope>import</scope> </dependency>

The Enforcer plugin discovered another transitive dependency of netty-transport-native-epoll IIRC. Yes Netty is fragmented, not modular.

@johnou Is this a kitchensink import? We want to include only what's needed.

@slaskawi I see, it's a new module in 4.1.11+

@rvansa You probably mean 9.1.11. Yes.

@rvansa no, as bom's are defined in dependencyManagement only the metadata is pulled in, you still need to define every dependency you want to use but you can omit the version.

See netty/netty#5994 (comment) for a good explanation.

@johnou OK, thanks for explanation. Yes, in this case importing the bom would make sense. @slaskawi Would you like to include that change here?

@slaskawi No, I mean new module in netty 4.1.11.

@slaskawi if you are using native transports you might want to wait until the next Netty release, see netty/netty#7563

@johnou Is this a kitchensink import? We want to include only what's needed.

dependencyManagement != dependencies

Whatever goes into dependencyManagement is not dependent upon necessarily. You have to then select what you depend on. We encourage users to do that all the time with Infinispan's bom (here).

I agree with you guys - let's use the BOM. However I wasn't able to do a full cleanup. The Karaf feature from persistence/remote pull in client/hotrod, which in turn uses Maven filtering to fill out its features.xml file. I believe this can be fixed by using Netty as bundles and not wrapping them. I created ISPN-8735 to investigate this option. Any volunteers?

tristantarrant · 2018-01-29T08:38:04Z

4.1.20 with a fix for netty/netty#7563 is out. Please update the PR to give CI a chance

johnou · 2018-01-29T09:34:38Z

@tristantarrant depending on your usage of Netty netty/netty#7627 might be considered a show stopper too (we haven't been able to update for the past two releases). 4.1.21 coming this week.

slaskawi · 2018-01-30T11:27:32Z

@tristantarrant @johnou With the latest changes, we pull whichever transport the BOM points us to. I would like to keep it that way.

johnou · 2018-01-30T13:21:59Z

@slaskawi the BOM doesn't dictate what transport is used, what I am getting at is for the past two Netty releases there have been two nasty regressions which may cause problems with projects which integrate with infinispan. Netty 4.1.21 is coming this week containing the fixes.

slaskawi · 2018-01-30T13:28:29Z

@johnou Could you please point out, which regressions are you talking about?

Also, could you please explain what artifacts do you mean by saying transport? If you mean netty-transport-native-epoll, which we are using in Infinispan, your statement is incorrect - Netty BOM does declare versions of those artifacts. Please have a look at this section.

johnou · 2018-01-30T13:51:15Z

@slaskawi the regressions are in my previous comments in this pull request #5676 (comment) which links to netty/netty#7563 that may cause OOO messages and general data corruption when using native transports and #5676 (comment) linking to netty/netty#7627 which can result in the JVM hanging on shutdown. As far as transports goes, in order to use epoll you are required to setup an epoll event loop, programatically, adding the epoll artifact to your classpath isn't enough, I am well aware of how a BOM works (I was the author of it in the Netty project netty/netty#6412) and I believe it is useful in this use-case which is why I suggested it.

gustavocoding · 2018-01-30T13:53:24Z

I think we should wait for 4.1.21

johnou · 2018-01-30T14:11:35Z

bom/pom.xml

@@ -666,62 +665,6 @@
            <artifactId>reactive-streams</artifactId>
            <version>${version.reactivestreams}</version>
         </dependency>
-         <dependency>


The dependencies should still be defined, just excluding the version tag.

Definitely not, it's a DependencyManagement not a Dependencies section.

h0h0 my bad, so it is, I should have looked at the full diff 👍

slaskawi · 2018-01-30T14:19:49Z

@tristantarrant Your call on waiting till 4.1.21 is out?

tristantarrant · 2018-01-30T14:20:21Z

Sure, let's wait

tristantarrant · 2018-01-30T14:20:36Z

Just wanted to catch any regressions (if any) with 4.1.20

slaskawi · 2018-01-30T14:22:45Z

@johnou Yes, that's exactly what we do (defining Epoll Event Loops and declaring Epoll Transport as a dependency). See the Server Core module and NettyTransport class.

johnou · 2018-01-30T14:38:52Z

Yes I figured so, I was just explaining my statement that you said was incorrect 😄

slaskawi · 2018-01-31T13:31:48Z

@johnou Ok, got it. Thanks for the explanation!

johnou · 2018-02-05T21:01:15Z

@slaskawi Netty 4.1.21 is now available in Central Maven.

slaskawi · 2018-02-06T10:08:42Z

@johnou Thanks a lot! I've updated this PR.

johnou · 2018-02-06T10:11:09Z

pom.xml

@@ -316,6 +315,9 @@
      <version.webdav.servlet>2.0.1</version.webdav.servlet>
      <version.weld-se>2.3.4.Final</version.weld-se>
      <version.weld>2.3.4.Final</version.weld>
+      <version.netty-tcpnative-boringssl-static>2.0.7.Final</version.netty-tcpnative-boringssl-static>


typo, tcpnative -> tcnative

Fixed. Thanks!

gustavocoding · 2018-02-07T12:16:35Z

CI is having trouble with this PR

slaskawi · 2018-02-08T09:05:13Z

@gustavonalle Yes, it seems the slave got a timeout.... Let me restart all of them and retrigger the job.

tristantarrant · 2018-02-15T08:50:29Z

Retriggering build

tristantarrant · 2018-02-15T12:21:35Z

Ok, it seems like it is hanging in the OSGi testsuite.

slaskawi · 2018-02-15T12:24:28Z

@tristantarrant I got out the loop for a while. Is it related or not?

tristantarrant · 2018-02-15T13:45:36Z

Yes: you have excluded org.conscrypt but something still wants it.

018-02-15T14:26:16,601 | ERROR | pool-1-thread-2 | BootFeaturesInstaller | 10 - org.apache.karaf.features.core - 4.1.4 | Error installing boot features
org.osgi.service.resolver.ResolutionException: Unable to resolve root: missing requirement [root] osgi.identity; osgi.identity=infinispan-embedded; type=karaf.feature; version="[9.2.0.SNAPSHOT,9.2.0.SNAPSHOT]"; filter:="(&(osgi.identity=infinispan-embedded)(type=karaf.feature)(version>=9.2.0.SNAPSHOT)(version<=9.2.0.SNAPSHOT))" [caused by: Unable to resolve infinispan-embedded/9.2.0.SNAPSHOT: missing requirement [infinispan-embedded/9.2.0.SNAPSHOT] osgi.identity; osgi.identity=org.infinispan.embedded; type=osgi.bundle; version="[9.2.0.SNAPSHOT,9.2.0.SNAPSHOT]"; resolution:=mandatory [caused by: Unable to resolve org.infinispan.embedded/9.2.0.SNAPSHOT: missing requirement [org.infinispan.embedded/9.2.0.SNAPSHOT] osgi.wiring.package; filter:="(osgi.wiring.package=org.conscrypt)"]]
at org.apache.felix.resolver.ResolutionError.toException(ResolutionError.java:42) ~[?:?]
at org.apache.felix.resolver.ResolverImpl.doResolve(ResolverImpl.java:391) ~[?:?]
at org.apache.felix.resolver.ResolverImpl.resolve(ResolverImpl.java:377) ~[?:?]
at org.apache.felix.resolver.ResolverImpl.resolve(ResolverImpl.java:349) ~[?:?]
at org.apache.karaf.features.internal.region.SubsystemResolver.resolve(SubsystemResolver.java:218) ~[?:?]
at org.apache.karaf.features.internal.service.Deployer.deploy(Deployer.java:291) ~[?:?]
at org.apache.karaf.features.internal.service.FeaturesServiceImpl.doProvision(FeaturesServiceImpl.java:1248) ~[?:?]
at org.apache.karaf.features.internal.service.FeaturesServiceImpl.lambda$doProvisionInThread$1(FeaturesServiceImpl.java:1147) ~[?:?]
at java.util.concurrent.FutureTask.run(FutureTask.java:266) [?:?]
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [?:?]
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [?:?]
at java.lang.Thread.run(Thread.java:748) [?:?]

slaskawi · 2018-02-15T13:52:21Z

Thanks a lot @tristantarrant! I will investigate this in the first free time slot.

johnou · 2018-02-15T14:00:21Z

org.conscrypt is optional but it's possible that it wasn't marked as optional in the Netty osgi bundle?

slaskawi · 2018-02-15T14:14:40Z

Rebased and excluded org.conscrypt from the embedded. Thanks @tristantarrant for sorting it out!

tristantarrant · 2018-02-15T16:17:47Z

Merged, thanks

rvansa reviewed Jan 15, 2018

View reviewed changes

tristantarrant added the Changes Required label Jan 29, 2018

slaskawi force-pushed the ISPN-8657/Upgrade_netty branch from 0ea8fc5 to 3716f88 Compare January 30, 2018 11:17

slaskawi removed the Changes Required label Jan 30, 2018

slaskawi added this to the 9.2.0.CR2 milestone Jan 30, 2018

johnou suggested changes Jan 30, 2018

View reviewed changes

gustavocoding modified the milestones: 9.2.0.CR2, 9.2.0.Final Jan 30, 2018

slaskawi force-pushed the ISPN-8657/Upgrade_netty branch from 3716f88 to 3e827d7 Compare February 6, 2018 10:07

slaskawi changed the title ~~ISPN-8657 Netty upgrade to 4.1.19~~ ISPN-8657 Netty upgrade to 4.1.21 Feb 6, 2018

slaskawi force-pushed the ISPN-8657/Upgrade_netty branch from 3e827d7 to 23d56a4 Compare February 6, 2018 10:08

johnou suggested changes Feb 6, 2018

View reviewed changes

slaskawi force-pushed the ISPN-8657/Upgrade_netty branch from 23d56a4 to cf42007 Compare February 6, 2018 10:47

johnou approved these changes Feb 6, 2018

View reviewed changes

tristantarrant added the Changes Required label Feb 15, 2018

ISPN-8657 Netty upgrade to 4.1.21

b7f9c82

slaskawi force-pushed the ISPN-8657/Upgrade_netty branch from cf42007 to b7f9c82 Compare February 15, 2018 14:14

tristantarrant merged commit c268f9c into infinispan:master Feb 15, 2018

ISPN-8657 Netty upgrade to 4.1.21 #5676

ISPN-8657 Netty upgrade to 4.1.21 #5676

Conversation

slaskawi commented Jan 15, 2018

Choose a reason for hiding this comment

johnou Jan 15, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tristantarrant commented Jan 29, 2018

johnou commented Jan 29, 2018 • edited Loading

slaskawi commented Jan 30, 2018

johnou commented Jan 30, 2018

slaskawi commented Jan 30, 2018

johnou commented Jan 30, 2018 • edited Loading

gustavocoding commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slaskawi commented Jan 30, 2018

tristantarrant commented Jan 30, 2018

tristantarrant commented Jan 30, 2018

slaskawi commented Jan 30, 2018

johnou commented Jan 30, 2018

slaskawi commented Jan 31, 2018

johnou commented Feb 5, 2018 • edited Loading

slaskawi commented Feb 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gustavocoding commented Feb 7, 2018

slaskawi commented Feb 8, 2018 • edited Loading

tristantarrant commented Feb 15, 2018

tristantarrant commented Feb 15, 2018

slaskawi commented Feb 15, 2018

tristantarrant commented Feb 15, 2018

slaskawi commented Feb 15, 2018

johnou commented Feb 15, 2018

slaskawi commented Feb 15, 2018

tristantarrant commented Feb 15, 2018

johnou Jan 15, 2018 •

edited

Loading

johnou commented Jan 29, 2018 •

edited

Loading

johnou commented Jan 30, 2018 •

edited

Loading

johnou commented Feb 5, 2018 •

edited

Loading

slaskawi commented Feb 8, 2018 •

edited

Loading