You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pekko.apache.org by GitBox <gi...@apache.org> on 2022/11/04 09:46:11 UTC
[GitHub] [incubator-pekko-http] jrudolph opened a new issue, #1: Move to upstream parboiled2 dependency
jrudolph opened a new issue, #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1
Akka HTTP right now contains a copy of parboiled2 in its sources (with some slight additions). This decision was originally made on the grounds that Akka should not have many dependencies on Scala libraries, so it can be released quickly after a new (incompatible) Scala version has been released.
By now, parboiled2 does not depend on shapeless any more and the ecosystem has more stabilized (and after all, maintaining the fork has not helped at all with migration to new Scala versions...). So, I'd suggest we move to upstream parboiled2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pekko.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pekko.apache.org
For additional commands, e-mail: dev-help@pekko.apache.org
[GitHub] [incubator-pekko-http] Seetaramayya commented on issue #1: Move to upstream parboiled2 dependency
Posted by "Seetaramayya (via GitHub)" <gi...@apache.org>.
Seetaramayya commented on issue #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1432944872
> > #### Approach 1: As Is approach (convert to dependency in `1.1.0`)
> > (+) As @spangaer mentioned, we can reason about bugs (if there are any) `1.0.0` does not do anything apart from different packaging. No need to worry about performance impacts. (+) No need to run heavy performance benchmarks before releasing (-) As @pjfanning mentioned, licence files management is difficult (I really don't know how painful this point)
> > #### Approach 2: Convert to dependency right now (in `1.0.0`)
> > (+) Licence file management would be easy (-) IMHO: Performance bench marks needs to be run before releasing (-) Assume underlying dependency `parboiled` has (`functional` or `performance` ) bug which resulted lot of side effects in the system narrow down the issue would be bit more complicated than the `Approach 1`
> > I would prefer `Approach 1` unless there is a huge impact on licensing part (I don't have knowledge in the licensing area). IMHO: With this approach we are separating the concerns
>
> @Seetaramayya there is momentum behind Approach 2. Would you have time to run the benchmarks in `http-bench-jmh` with and without this change? I think we can hold off for a few days regardless. I may have time in a few days to try it myself.
I would love to do that, but I joined the conversation bit late. @jrudolph provided the results
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] spangaer commented on issue #1: Move to upstream parboiled2 dependency
Posted by "spangaer (via GitHub)" <gi...@apache.org>.
spangaer commented on issue #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1428445791
If there's no functional difference by using upstream, I don't think switching to the dependency is real violation of the v1.0 intent.
As long as Pekko 1.0.0 exhibits the same behavior as Akka 2.6.x feature for feature, bug for bug after the rename refactor in consuming code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] spangaer commented on issue #1: Move to upstream parboiled2 dependency
Posted by "spangaer (via GitHub)" <gi...@apache.org>.
spangaer commented on issue #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1429353233
For clarity, I favor approach 2 (do it in 1.0.0). I do believe some validation should be made, but best effort only, it shouldn't absorb all energy either, otherwise there's nothing to be gained.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph closed issue #1: Move to upstream parboiled2 dependency
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph closed issue #1: Move to upstream parboiled2 dependency
URL: https://github.com/apache/incubator-pekko-http/issues/1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] Seetaramayya commented on issue #1: Move to upstream parboiled2 dependency
Posted by "Seetaramayya (via GitHub)" <gi...@apache.org>.
Seetaramayya commented on issue #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1429319060
## Approach 1: As Is approach (convert to dependency in `1.1.0`)
(+) As @spangaer mentioned, we can reason about bugs (if there are any) `1.0.0` does not do anything apart from different packaging. No need to worry about performance impacts.
(+) No need to run heavy performance benchmarks before releasing
(-) As @pjfanning mentioned, licence files management would be easy (I really don't know how painful)
## Approach 2: Convert to dependency right now (in `1.0.0`)
(+) Licence file management would be easy
(-) IMHO: Performance bench marks needs to be run before releasing
(-) Assume underlying dependency `parboiled` has (`functional` or `performance` ) bug which resulted lot of side effects in the system narrow down the issue would be bit more complicated than the approach (I am talking from relative point of view).
I would prefer `Approach 1` unless there is a huge impact on licensing part (I don't know have knowledge in the area). IMHO: With this approach we are separating the concerns
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on issue #1: Move to upstream parboiled2 dependency
Posted by GitBox <gi...@apache.org>.
pjfanning commented on issue #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1335298656
@jrudolph do you think this is best done now or should we wait till after the v1.0.0 release?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on issue #1: Move to upstream parboiled2 dependency
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on issue #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1432927096
Regarding merging now, I personally don't free that strongly on this. My general point is that I don't think there is a rush to merge this PR (not expecting any merge conflicts in the later) and unlike pekko core, a release of pekko-http is further down the line, i.e. I am also not seeing us needing to do the necessary license changes for this repo anytime soon (mainly because we have much bigger fish to fry).
PR is approved on my end, so if people feel that its overall wiser to merge it now then good with me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on issue #1: Move to upstream parboiled2 dependency
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on issue #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1432895992
FTR:
Run with `jmh:run -r3 -i3 -wi 3 -w 3 ServerProcessingBenchmark`
Before:
```
[info] Result "org.apache.pekko.http.impl.engine.ServerProcessingBenchmark.benchRequestProcessing":
[info] 327842.154 ±(99.9%) 3747.035 ops/s [Average]
[info] (min, avg, max) = (324466.534, 327842.154, 331264.288), stdev = 2229.801
[info] CI (99.9%): [324095.118, 331589.189] (assumes normal distribution)
```
After:
```
[info] Result "org.apache.pekko.http.impl.engine.ServerProcessingBenchmark.benchRequestProcessing":
[info] 309453.217 ±(99.9%) 4596.546 ops/s [Average]
[info] (min, avg, max) = (304411.018, 309453.217, 312560.405), stdev = 2735.331
[info] CI (99.9%): [304856.671, 314049.763] (assumes normal distribution)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on issue #1: Move to upstream parboiled2 dependency
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on issue #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1432878256
I see an ~ 5-10% slowdown in `ServerProcessingBenchmark`. There are a few optimization commits that we are losing now that we should try to upstream:
* d3b1322d4526171370935ec7b4648bc45fabc672
* 1a50f8afd1f21b2c686b8015034a12a1fed47ce1
* 58d8f48f2bb81f3eedbc743aa890dc8e80da252f
* 14f5ae3cbb50240b25f60dda8182284a0776d1a2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] spangaer commented on issue #1: Move to upstream parboiled2 dependency
Posted by "spangaer (via GitHub)" <gi...@apache.org>.
spangaer commented on issue #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1433345810
Probably true that there's no hurry yet, but I do tend to agree that the performance penalty is acceptable for a first version. Should it come to that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on issue #1: Move to upstream parboiled2 dependency
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on issue #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1441881302
Done in #14
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on issue #1: Move to upstream parboiled2 dependency
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on issue #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1427895592
There is some discussion about bringing this into v1.0.0 release in https://github.com/apache/incubator-pekko-http/issues/39.
It would simplify the license files in pekko-http if we just used the parboiled2 jar instead of embedding the source in our code base.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on issue #1: Move to upstream parboiled2 dependency
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on issue #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1429388380
The main reasoning should be about compatibility:
* staying with the vendor copy allows users to use a parboiled2 version independently of pekko-http (currently not a huge concern)
* some users of akka-http might have accidentally relied on the akka-parsing artifact and might be confused if the classes are now missing (not a concern as the parsing infrastructure is meant to be internal, and also package names are not compatible with akka anyway)
* related to our original reasoning to vendor, in the future, we need to consider whether a parboiled2 dependency will hold us up with adopting new Scala versions because we might have to always wait for a compatible parboiled2 version before being able to move to a new Scala 3 version (IMO, we can expect some problems with compatibility in the future but it stays questionable if we could fix them more easily in our vendor copy than upstream)
All in all, I'd say moving now is the best choice because we are breaking compatibility anyway and the risk of breaking something in a major way is relatively small.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] nvollmar commented on issue #1: Move to upstream parboiled2 dependency
Posted by GitBox <gi...@apache.org>.
nvollmar commented on issue #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1303188909
That would make sense
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pekko.apache.org
For additional commands, e-mail: dev-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on issue #1: Move to upstream parboiled2 dependency
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on issue #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1431386075
> The main reasoning should be about compatibility:
>
> * staying with the vendor copy allows users to use a parboiled2 version independently of pekko-http (currently not a huge concern)
> * some users of akka-http might have accidentally relied on the akka-parsing artifact and might be confused if the classes are now missing (not a concern as the parsing infrastructure is meant to be internal, and also package names are not compatible with akka anyway)
> * related to our original reasoning to vendor, in the future, we need to consider whether a parboiled2 dependency will hold us up with adopting new Scala versions because we might have to always wait for a compatible parboiled2 version before being able to move to a new Scala 3 version (IMO, we can expect some problems with compatibility in the future but it stays questionable if we could fix them more easily in our vendor copy than upstream)
>
> All in all, I'd say moving now is the best choice because we are breaking compatibility anyway and the risk of breaking something in a major way is relatively small.
I also agree with this sentiment. Regarding the point about being held back on upstream if they are slow to adopt features (such as newer Scala release), even in the worst case scenario its possible to fork that library (with all of its original licenses) to do such changes and use the fork until it gets merged to upstream.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on issue #1: Move to upstream parboiled2 dependency
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on issue #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1432153130
> #### Approach 1: As Is approach (convert to dependency in `1.1.0`)
> (+) As @spangaer mentioned, we can reason about bugs (if there are any) `1.0.0` does not do anything apart from different packaging. No need to worry about performance impacts. (+) No need to run heavy performance benchmarks before releasing (-) As @pjfanning mentioned, licence files management is difficult (I really don't know how painful this point)
>
> #### Approach 2: Convert to dependency right now (in `1.0.0`)
> (+) Licence file management would be easy (-) IMHO: Performance bench marks needs to be run before releasing (-) Assume underlying dependency `parboiled` has (`functional` or `performance` ) bug which resulted lot of side effects in the system narrow down the issue would be bit more complicated than the `Approach 1`
>
> I would prefer `Approach 1` unless there is a huge impact on licensing part (I don't have knowledge in the licensing area). IMHO: With this approach we are separating the concerns
@Seetaramayya there is momentum behind Approach 2. Would you have time to run the benchmarks in `http-bench-jmh` with and without this change? I think we can hold off for a few days regardless. I may have time in a few days to try it myself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] pjfanning commented on issue #1: Move to upstream parboiled2 dependency
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on issue #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1434436483
relates to #35
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] kw217 commented on issue #1: Move to upstream parboiled2 dependency
Posted by "kw217 (via GitHub)" <gi...@apache.org>.
kw217 commented on issue #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1428206958
I agree, an explicit dependency is much better than taking a copy. This makes it much easier to take upgrades of the dependency (and to notice the need for them), which can be an issue for security.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on issue #1: Move to upstream parboiled2 dependency
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on issue #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1432922034
> If this is done before a release is made then we have the best of both worlds, otherwise we might have to deal with the pain/annoyance of license changes (not sure if the community would accept such a slowdown given that one of pekko-http's main selling points is for it to be fast).
I would be fine with merging this now and try to get the improvements back later. Since the benchmark is missing processing (no actual sockets in the benchmark), the actual slowdown is less than shown here. I have some more ideas how to improve parsing which would change the story again, so maybe less important to keep single-digit optimizations right now if we can simplify the build and get a release out sooner.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on issue #1: Move to upstream parboiled2 dependency
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on issue #1:
URL: https://github.com/apache/incubator-pekko-http/issues/1#issuecomment-1432884633
To me it makes sense to leave the PR for now and try to upstream the necessary changes as soon as we can to see if upstream can land those optimizations + release them.
If this is done before a release is made then we have the best of both worlds, otherwise we might have to deal with the pain/annoyance of license changes (not sure if the community would accept such a slowdown given that one of pekko-http's main selling points is for it to be fast).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org