moving to upstream parboiled2 dependency #14

luksow · 2022-12-10T13:02:00Z

Hi,

since #1 was rather easy, I took stab at it. The parboiled2 interface changed a bit over the time, so more changes were needed than just the imports.

Thing to consider is to remove akka-parsing completely but I think it was partially sorted out in https://github.com/apache/incubator-pekko-http/tree/scala-3 branch

pjfanning · 2022-12-10T21:43:33Z

Thanks for the PR. We still need to decide if we want to make this change yet. For v1.0.0, we generally want minimal changes.
The scala-3 branch is shaping up to be the basis of the v1.1.0 release and we may end targeting this work to that release and possibly that branch.
The scalfmt checks are failing and it would be a good idea to change the order of the imports so that the org.parboiled imports are not with the Akka imports.

luksow · 2022-12-11T10:58:15Z

@pjfanning thanks for your comments. I did scalafmt and also reorganized imports so that org.parboiled2 is always after akka's.

After you decide the faith of v1.0.0 - let me know. I'm happy to target v1.1.0 or scala-3 branch instead.

pjfanning · 2022-12-11T19:45:36Z

@mdedetrich, @jrudolph, @nvollmar - I think we should consider treating the scala-3 branch as the v1.1.0 branch. If this PR was retargeted to the scala-3 branch, would any of you support approving the PR?

nvollmar · 2022-12-12T06:24:10Z

I think that would make sense.

mdedetrich · 2022-12-12T07:17:37Z

I would also support putting this into 1.1.x, will retarget the PR when the 1.1.x branch is out. @luksow Thanks a lot for the work, much appreciated!

pjfanning · 2023-02-14T11:16:50Z

@luksow would you consider updating this PR to work with the latest main branch? The akka package classes have moved to org.apache.pekko. The main branch should be more stable now.

We are actually thinking about merging this into main for the v1.0.0 release.

mdedetrich · 2023-02-14T11:18:53Z

@luksow would you consider updating this PR to work with the latest main branch? The akka package classes have moved to org.apache.pekko. The main branch should be more stable now.

We are actually thinking about merging this into main for the v1.0.0 release.

I was also going to mention this. There are still a couple more big structural changes that need to be done (i.e. configuration change from akka to pekko as well as renaming types that have the akka in them). I don't think that these changes would cause further merge conflicts for this kind of PR but I am going to focus on this for the week.

jrudolph · 2023-02-14T11:36:45Z

pekko-http-core/src/main/scala/akka/http/impl/model/parser/HeaderParser.scala

@@ -108,25 +108,25 @@ private[http] object HeaderParser {

  object EmptyCookieException extends SingletonException("Cookie header contained no parsable cookie values.")

-  def lookupParser(headerName: String, settings: Settings = DefaultSettings): Option[String => HeaderParser#Result] =
-    dispatch.lookup(headerName).map { runner => (value: String) =>


We should keep this optimization, maybe we have to keep at our own version of DynamicRuleDispatch?

Maybe, let's try to upstream the optimization and go back to the simpler version for now (which adds an extra hashmap lookup per header parsed).

Makes sense to me

Maybe, let's try to upstream the optimization and go back to the simpler version for now (which adds an extra hashmap lookup per header parsed).

If we do this then lets make an issue on it.

Created #45

jrudolph · 2023-02-14T11:56:23Z

I added a merge commit to fix the conflicts.

mdedetrich · 2023-02-14T11:57:51Z

I added a merge commit to fix the conflicts.

Thanks, I just quickly checked the PR and I don't see any class files containing Akka being modified so there shouldn't be any major merge conflicts later

jrudolph · 2023-02-14T11:59:36Z

http-core/src/main/scala/org/apache/pekko/http/impl/model/parser/CacheControlHeader.scala

 import pekko.http.scaladsl.model.headers._
 import CacheDirectives._

-private[parser] trait CacheControlHeader { this: Parser with CommonRules with CommonActions with StringBuilding =>
+private[parser] trait CacheControlHeader { this: HeaderParser =>


Somehow the Scala compiler got confused here about the more restricted self-type, not sure what's the issue, but that works for now.

jrudolph · 2023-02-14T12:01:06Z

http-core/src/main/scala/org/apache/pekko/http/impl/model/parser/HeaderParser.scala

@@ -110,11 +110,11 @@ private[http] object HeaderParser {
  object EmptyCookieException extends SingletonException("Cookie header contained no parsable cookie values.")

  def lookupParser(headerName: String, settings: Settings = DefaultSettings): Option[String => HeaderParser#Result] =
-    dispatch.lookup(headerName).map { runner => (value: String) =>
-      import pekko.parboiled2.EOI
+    Some { (value: String) =>


I changed that to keep the old signatures even if the new one might "lie" because it may still fail later with a RuleNotFound exception (only if we are adding ModeledHeaders without accompanying it with the correctly named parser).

jrudolph

lgtm but has to wait for a decision in #1.

jrudolph · 2023-02-14T12:18:15Z

(Fixed the test fixes I accidentally reverted from @luksow's state)

luksow · 2023-02-14T15:38:59Z

Wow, you're moving quick! I barely was able to have a look and all work is already done :) Glad to see it moving forward.

LGTM.

mdedetrich · 2023-02-14T15:40:15Z

Wow, you're moving quick! I barely was able to have a look and all work is already done :) Glad to see it moving forward.

Yeah this is a common problem we have....

There is still one issue though, which is that the PR is unable to be merged because it needs to be rebased due to conflicts (at least thats what github is complaining)

luksow · 2023-02-14T15:43:38Z

@mdedetrich: here it says it's good to go 🤔

mdedetrich · 2023-02-14T15:58:57Z

I need to download your version of github :D

spangaer · 2023-02-15T08:23:00Z

Tie breaker

Maybe you should try light mode 😉

Perhaps it's resolved by now, I saw the same yesterday evening, but it's not the first time I notice GitHub being "eventually consistent"

mdedetrich · 2023-02-15T09:09:57Z

So I noticed that on my phone its fine and on the desktop its still broken, so this is probably the most liberal definition of eventual that I have seen in a while.

Anyways it just means someone else will have to merge.

jrudolph · 2023-02-15T09:52:05Z

The reason is probably that you selected "rebase and merge" which isn't possible cleanly due to my merge commit. But we can squash here.

mdedetrich · 2023-02-15T13:06:04Z

The reason is probably that you selected "rebase and merge" which isn't possible cleanly due to my merge commit. But we can squash here.

You are right, this is a case of UI ergonomics because the drop down button which lets you change the merge strategy was kind of greyed out (implying it was disabled), probably due to it being mixed with dark theme did not help.

mdedetrich

lgtm

pjfanning · 2023-02-15T22:34:15Z

could we hold off for a couple of days? - on #1, it has been suggested that we run a perf test - may be over cautious but may be no harm if someone has time to run the benchmarks in this project (with and without this change)

mdedetrich · 2023-02-15T22:35:31Z

Sure

mdedetrich · 2023-02-16T10:47:41Z

As per discussion at #1 (comment) I have applied the upstream label (indicating that the PR is waiting for some upstream changes).

jrudolph · 2023-02-22T08:54:14Z

Cleaned up the commits into a single one based on top of current master.

jrudolph · 2023-02-22T08:56:02Z

As per discussion at #1 (comment) I have applied the upstream label (indicating that the PR is waiting for some upstream changes).

I don't think we should hold off for any upstream changes as discussed above and in #1, the performance hit is currently minor and can partly be solved without upstream changes.

pjfanning

lgtm - the perf hit is small

pjfanning · 2023-02-22T16:15:58Z

@luksow this change is large enough that we probably need a CLA. If you don't have one already, could you get an iCLA as documented in https://www.apache.org/licenses/contributor-agreements.html ?

jrudolph · 2023-02-23T10:33:08Z

@luksow this change is large enough that we probably need a CLA. If you don't have one already, could you get an iCLA as documented in apache.org/licenses/contributor-agreements.html ?

AFAIU, we have to collect CLAs for every contributor. We have the problem here that if we don't get a CLA we will have to resubmit the exact same PR with a new author because it is so trivial that there is no other alternative to these exact changes. After all only 66 lines were updated or added here.

pjfanning · 2023-02-23T10:42:09Z

In practice, ASF projects don't collect CLAs from every user who submits a PR. There is no magic criterion for what makes a PR significant enough to require one but the PR changes 70 files, even if the changes are not very large.

Let's see what @luksow says, if he doesn't have time to sort out the CLA, we are probably going to have to look at alternatives.

luksow · 2023-02-23T11:08:35Z

@pjfanning @jrudolph I've just signed and sent ICLA.

mdedetrich · 2023-02-23T12:14:55Z

In practice, ASF projects don't collect CLAs from every user who submits a PR. There is no magic criterion for what makes a PR significant enough to require one but the PR changes 70 files, even if the changes are not very large.

My understanding is also the same, an ICLA is required for significant contributions but the definition of this is entirely subjective.

mdedetrich added this to the 1.1.0 milestone Dec 12, 2022

This was referenced Feb 13, 2023

consider using parboiled2 as a jar dependency in v1.0.0 #39

Closed

Consider putting akka.parboiled2.util.Base64 into its own sbt module/artifact #40

Closed

jrudolph reviewed Feb 14, 2023

View reviewed changes

jrudolph approved these changes Feb 14, 2023

View reviewed changes

jrudolph mentioned this pull request Feb 14, 2023

Upstream former DynamicRuleDispatch.lookup optimization to parboiled2 #45

Closed

jrudolph force-pushed the main branch from e6e0fcc to d69f6ca Compare February 14, 2023 12:17

mdedetrich modified the milestones: 1.1.0, 1.0.0 Feb 14, 2023

mdedetrich approved these changes Feb 15, 2023

View reviewed changes

mdedetrich added the upstream Blocked by upstream for some reason label Feb 16, 2023

moving to upstream parboiled2 dependency (apache#1)

70b16d0

jrudolph force-pushed the main branch from e14bce9 to 70b16d0 Compare February 22, 2023 08:53

pjfanning approved these changes Feb 22, 2023

View reviewed changes

jrudolph merged commit 26846a0 into apache:main Feb 23, 2023

jrudolph mentioned this pull request Feb 23, 2023

Move to upstream parboiled2 dependency #1

Closed

mdedetrich mentioned this pull request Mar 4, 2023

Track upstreamed performance changes for parboiled2 #103

Closed

pjfanning added the release notes should be mentioned in release notes label Jun 11, 2023

mdedetrich mentioned this pull request Jun 19, 2023

Remove unneeded parser settings #189

Merged

moving to upstream parboiled2 dependency #14

moving to upstream parboiled2 dependency #14

Conversation

luksow commented Dec 10, 2022

pjfanning commented Dec 10, 2022

luksow commented Dec 11, 2022

pjfanning commented Dec 11, 2022

nvollmar commented Dec 12, 2022

mdedetrich commented Dec 12, 2022

pjfanning commented Feb 14, 2023

mdedetrich commented Feb 14, 2023 • edited

jrudolph Feb 14, 2023

Choose a reason for hiding this comment

jrudolph Feb 14, 2023

Choose a reason for hiding this comment

mdedetrich Feb 14, 2023

Choose a reason for hiding this comment

mdedetrich Feb 14, 2023

Choose a reason for hiding this comment

jrudolph Feb 14, 2023

Choose a reason for hiding this comment

jrudolph commented Feb 14, 2023

mdedetrich commented Feb 14, 2023

jrudolph Feb 14, 2023

Choose a reason for hiding this comment

jrudolph Feb 14, 2023

Choose a reason for hiding this comment

jrudolph left a comment

Choose a reason for hiding this comment

jrudolph commented Feb 14, 2023

luksow commented Feb 14, 2023

mdedetrich commented Feb 14, 2023

luksow commented Feb 14, 2023

mdedetrich commented Feb 14, 2023

spangaer commented Feb 15, 2023

mdedetrich commented Feb 15, 2023 • edited

jrudolph commented Feb 15, 2023

mdedetrich commented Feb 15, 2023 • edited

mdedetrich left a comment

Choose a reason for hiding this comment

pjfanning commented Feb 15, 2023

mdedetrich commented Feb 15, 2023

mdedetrich commented Feb 16, 2023

jrudolph commented Feb 22, 2023

jrudolph commented Feb 22, 2023

pjfanning left a comment

Choose a reason for hiding this comment

pjfanning commented Feb 22, 2023

jrudolph commented Feb 23, 2023

pjfanning commented Feb 23, 2023

luksow commented Feb 23, 2023

mdedetrich commented Feb 23, 2023

mdedetrich commented Feb 14, 2023 •

edited

mdedetrich commented Feb 15, 2023 •

edited

mdedetrich commented Feb 15, 2023 •

edited