Artemis 3163 #15
Artemis 3163 #15
Conversation
Remove reflection and use the netty binaries just as we do with epoll and kqueue
Add documents ensuring it has a clear note about incubator status
There was a problem hiding this comment.
If the dependency is going to be added as a hard dep and distributed (I dont think it should be) then I definitely think it should have a minimal test added to exercise it. I also think that if it isnt though.
The builds failed due to checkstyle. I also see it is also very out of date, so seems like a rebase is still needed after this (I appreciate why you probably didnt do it here).
| <dependency> | ||
| <groupId>io.netty.incubator</groupId> | ||
| <artifactId>netty-incubator-transport-native-io_uring</artifactId> | ||
| <classifier>${netty-transport-native-io_uring-classifier}</classifier> | ||
| </dependency> |
There was a problem hiding this comment.
I think this should be optional for now until closer to the point its considered non experimental and has more than zero testing. I expect few to use it initially, and those who do can easily add it themselves, so it seems premature to make it a hard dependency for everyone.
There was a problem hiding this comment.
Whilst i agree to testing. I disagree with not including the jar. Its behind a feature toggle, by not including it youre making it much tricker for users to test/try and give feedback on. Even myself whilst i can package and deploy to lower envs the apache dist for better real testing, and getting feedback i have to jump through alot of hoops to the point it makes it prohibitive to do that and get that feedback loop if i have to customise by adding jars outside distros
There was a problem hiding this comment.
I would imagine it to be the same with softwaremill who run perf tests where they will just take the distro only. Which is one of the main targets we would be asking them to retest with this.
There was a problem hiding this comment.
I dont consider that an issue at this stage. I think its pretty trivial to customise things like this, little different to getting the distribution if thats what you do (many dont). Certainly not a good reason to include it, for me.
| @@ -86,6 +86,7 @@ | |||
| <include>org.apache.commons:commons-dbcp2</include> | |||
| <include>org.apache.commons:commons-pool2</include> | |||
| <include>io.netty:netty-all</include> | |||
| <include>io.netty.incubator:netty-incubator-transport-native-io_uring</include> | |||
There was a problem hiding this comment.
I similarly think its premature to distribute it.
There was a problem hiding this comment.
I agree, leave it on a branch till there is a non incubator netty dependency.
| @@ -113,7 +115,7 @@ | |||
| public static final String NIO_ACCEPTOR_TYPE = "NIO"; | |||
| public static final String EPOLL_ACCEPTOR_TYPE = "EPOLL"; | |||
| public static final String KQUEUE_ACCEPTOR_TYPE = "KQUEUE"; | |||
| public static final String IOURING_ACCEPTOR_TYPE = "IO_URING"; | |||
| public static final String IOURING_ACCEPTOR_TYPE = "IOURING"; | |||
There was a problem hiding this comment.
I think the existing name was fine, nicer even. Per the other PR, this doesnt seem to be used for URL config, with the other boolean toggle is, so I'm not sure it matters if it has the underscore, which is actually part of the io_uring name.
| > | ||
| > @see https://github.com/netty/netty-incubator-transport-io_uring | ||
|
|
||
| On supported platforms IO_Uring can be used @see https://en.wikipedia.org/wiki/Io_uring |
There was a problem hiding this comment.
Supported platforms could be less vague, i.e its Linux (EDIT: which is then mentioned below...it seems superfluous to have both).
I believe the name is simply io_uring rather than IO_Uring (applies here and most other places IO_Uring is used)
|
Will leave with you @franz1981. Upto you if you wish to merge and accept my changes or not. If it does get merged in with deps and jars then once released i can get it to our pre prod env and get some real use case stats on perf and resource reduction. Rather than just a simulation env. |
|
I'll take some time to decide, because both the points of @gemmellr and @michaelandrepearce are good ones :) |
No description provided.