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

Move RoutingSettings to akka-http module #2307

Merged
merged 4 commits into from Dec 12, 2018

Conversation

Projects
None yet
5 participants
@renatocaval
Copy link
Contributor

renatocaval commented Dec 6, 2018

This PR moves the RoutingSettings to the akka-http module.

This is currently in akka-http-core which is odd since the routing dsl is part of akka-http module.

The build will fail because of MiMa.

RoutingSettings is public API, although they are marked with @DoNotInherit user may be relying on them. That could be a problem if someone is using that class without also using akka-http jar. That would be odd, but technically possible.

Fot that we have three possible solutions:

  1. keep the old classes, mark them as deprecated and add new classes with a different name, eg: RoutingDslSettings?
  2. add the exclusion to MiMa, although they are not private or internal API, they are not expected to be used.
  3. drop this PR and keep it as is

We have run into an issue where mixed versions of akka-http would cause an exception at runtime, for instance: akka-http v10.0.13 mixed with akka-http-core v10.1.5.

Moving the class to akka-http is not a real fix, but I would consider it just a maintenance thing. Just to keep the code base coherent.

To solve the issue of having mixes akka-http, we should add a similar check like we now have in Akka.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Dec 6, 2018

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

decode-max-bytes-per-chunk = 1m
decode-max-size = 8m
file-io-dispatcher = ${akka.stream.blocking-io-dispatcher}
}

This comment has been minimized.

@renatocaval

renatocaval Dec 6, 2018

Contributor

this is the best example that that class was on the wrong place.

In this test we check for the settings equality. The test works perfectly for all settings except for the RoutingSettings. To make it work, we need to define the settings in place because they are not in the reference.conf here.

@renatocaval

This comment has been minimized.

Copy link
Contributor

renatocaval commented Dec 6, 2018

I tried to add a RoutingDslSettings, but that doesn't help much. The old RoutingSettings is passed to a few methods here and there so we can't trick MiMa on that.

So, either we give up on this PR or we add deprecated methods everywhere.

@raboof

This comment has been minimized.

Copy link
Member

raboof commented Dec 6, 2018

Would it be possible to move it to the akka-http project, but otherwise keep it binary compatible with the version that was in akka-http-core? that might be 'binary compatible' enough.

@renatocaval

This comment has been minimized.

Copy link
Contributor

renatocaval commented Dec 6, 2018

Yes, that's what this PR is doing. I will fix MiMa and we can let it rest for a while why we think about it.

@jlprat

This comment has been minimized.

Copy link
Member

jlprat commented Dec 6, 2018

OK TO TEST

@akka-ci akka-ci added the validating label Dec 6, 2018

@renatocaval

This comment has been minimized.

Copy link
Contributor

renatocaval commented Dec 6, 2018

@jlprat, the build won't pass because of MiMA.

@akka-ci akka-ci added needs-attention and removed validating labels Dec 6, 2018

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Dec 6, 2018

Test FAILed.

@jlprat

This comment has been minimized.

Copy link
Member

jlprat commented Dec 6, 2018

@renatocaval Yes I know, it was just to ask Jenkins to pick it up. I doesn't matter if it stays red for some days.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Dec 6, 2018

Test FAILed.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Dec 7, 2018

Test PASSed.

@raboof

This comment has been minimized.

Copy link
Member

raboof commented Dec 7, 2018

This seems safe enough to me. It may not be 'binary compatible' in the MiMa sense, but the reason we want to deliver bincompat is to make it possible for people to upgrade without fear, and here apart from the fact that any downstream projects that relied on RoutingSettings must also depend on akka-http (not just akka-http-core) they should be able to update just fine. Still curious why RoutingSettings was in core in the first place ;)

@renatocaval

This comment has been minimized.

Copy link
Contributor

renatocaval commented Dec 7, 2018

@raboof I didn't dig further to find out the reason, but I believe it's a leftover of some refactoring. That's just a guess.

@renatocaval renatocaval changed the title WIP: move RoutingSettings to akka-http module Move RoutingSettings to akka-http module Dec 10, 2018

@renatocaval

This comment has been minimized.

Copy link
Contributor

renatocaval commented Dec 10, 2018

@raboof, the issue we have when mixing 10.0.13 with 10.1.5, can also happen when mixing 10.0.13 with 10.0.14 because decode-max-size was backported.

df3b682#diff-18cb77b528e9f08fba0ba56e43119085

Since Lagom and Play only depend on akka-http-core (v.10.1.5), a user will hit it whenever akka-http is added transitively, eg: when adding reactive-lib which depends on akka-http v10.0.13

@renatocaval

This comment has been minimized.

Copy link
Contributor

renatocaval commented Dec 10, 2018

For the record,

Lagom 1.4.9 is using akka-http-core 10.0.14
Lagom 1.5.0 (upcoming) is using akka-http-core 10.1.5

At the end of the day, that's not an akka-http problem. But the fact that RountingSettings is on the "wrong" place make it fragile to for other libs or framework depending on it. And it breaks even between minor, bug fix, versions.

@raboof

This comment has been minimized.

Copy link
Member

raboof commented Dec 10, 2018

Problems are expected when different versions of akka-http artifacts are mixed - even between minor, bug fix versions - so that cannot be a motivation for this change.

@raboof

raboof approved these changes Dec 10, 2018

Copy link
Member

raboof left a comment

Thanks, I like this improvement - it makes things more consistent and easier to understand.

I think the chance of someone depending on only akka-http-core and not akka-http but still using RoutingSettings are slim, so that justifies the 'binary incompatibility' in akka-http-core here.

@jlprat

This comment has been minimized.

Copy link
Member

jlprat commented Dec 12, 2018

I think the chance of someone depending on only akka-http-core and not akka-http but still using RoutingSettings are slim, so that justifies the 'binary incompatibility' in akka-http-core here.

@raboof is it worth adding this note in the migration guide? (https://github.com/akka/akka-http/blob/master/docs/src/main/paradox/migration-guide/migration-guide-10.1.x.md)

@jrudolph
Copy link
Member

jrudolph left a comment

Good cleanup, LGTM.

I think it's unlikely that someone referenced those classes because - as witnessed by this PR - there are no references from just akka-http-core to them.

@jrudolph jrudolph merged commit d5a3798 into akka:master Dec 12, 2018

3 checks passed

Jenkins PR Validation Test PASSed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@jrudolph jrudolph added this to the 10.1.6 milestone Dec 12, 2018

@renatocaval renatocaval deleted the renatocaval:routing-settings branch Jan 7, 2019

@renatocaval

This comment has been minimized.

Copy link
Contributor

renatocaval commented Jan 7, 2019

The issue that triggered that fix was backported in 10.0.14. Should this be backported to 10.0.x branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment