Skip to content
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

Rename akka package to org.apache.pekko #34

Merged
merged 1 commit into from
Feb 14, 2023

Conversation

mdedetrich
Copy link
Contributor

@mdedetrich mdedetrich commented Feb 5, 2023

This PR renames the akka package to org.apache.pekko, as with the equivalent pekko core PR its recommended to checkout the branch and view the changes in a local git diff viewer since github UI does a bad job of rendering large diffs.

Resolves: #21

Notable changes

  • Due to pekko-http using private[akka], it wasn't possible to only change the pekko-http packages on a granular level. The PR also updates dependency resolution so it brings in the latest pekko snapshot that has been published into the Apache Nexus snapshots repo. Doing this involved making quite a few changes to AkkaBuild (which is now called PekkoBuild), this part of the code handles automatically bringing in latest snapshot from a repository. @jrudolph Maybe you want to look into this so there isn't anything fishy
  • In general I only tried to do changes to package names and not configurations. There were however some exceptions, i.e. since pekko-http references configurations in pekko, some typesafe configs had to be changed from akka. to pekko.. A good example of this is akka.actor which was changed to pekko.actor (since pekko.actor is not defined in pekko-http, its defined in pekko-core).
    • Ontop of this, some configurations also had to be split up because they were generally grouped under akka. Examples of this are H2SpecIntegrationSpec and DontLeakActorsOnFailingConnectionSpecs. This splitting of configuration will be reverted when I make a PR to also updated all of the pekko-http configurations
  • Paradox docs have also been updated for any references to packages and manually screened multiple times. I took care so that the import statements are idiomatic (like with equivalent pekko core PR, for scala sources we do import org.apache.pekko and then after we do the various import pekko...)

@mdedetrich mdedetrich marked this pull request as draft February 5, 2023 10:12
@mdedetrich mdedetrich force-pushed the rename-akka-package-to-pekko branch 8 times, most recently from d9e8059 to f9deacf Compare February 6, 2023 09:39
@mdedetrich
Copy link
Contributor Author

@jrudolph I have done most of the work in regards to the package name change however I am now encountering an issue where any test that uses org.apache.pekko.testkit.EventFilter along with org.apache.pekko.testkit.TestEvent.intercept is failing. I have dune a cursory look through the code and couldn't find anything that would hint as to why its not working (i.e. I can't find any kind of hardcoded checking for the akka package).

If there is anything on the top of your head that comes to mind let me know, otherwise I will have another look at this later in the week.

@jrudolph
Copy link
Contributor

jrudolph commented Feb 6, 2023

If there is anything on the top of your head that comes to mind let me know, otherwise I will have another look at this later in the week.

The event filters rely on pekko.loggers being set correctly, but settings have not yet been changed to use the pekko prefix yet. In this case, the problem is in AkkaSpecWithMaterializer.

@mdedetrich
Copy link
Contributor Author

mdedetrich commented Feb 6, 2023

Thanks for finding this out, indeed the issue is that some configuration in pekko core has changed from akka to pekko. Although its possible to find which specific typesafe config settings refer specifically to the parent pekko core, I think it will be easier/safer/clearer to also do all of the configuration changes in a separate commit in this PR.

If it is however only a few changes necessary to make tests pass I might decide just to do those, lets see.

@jrudolph
Copy link
Contributor

jrudolph commented Feb 6, 2023

Although its possible to find which specific typesafe config settings refer specifically to the parent pekko core

That should be easy enough, every setting that does not start with akka.http would be referring to a core setting.

@mdedetrich mdedetrich force-pushed the rename-akka-package-to-pekko branch 6 times, most recently from fc8a08c to 8f29425 Compare February 6, 2023 13:12
@mdedetrich
Copy link
Contributor Author

So good news, tests are finally passing. There are still a couple of things I need to double check, i.e. that the import statements in paradox are nicely formatted and some tests seem to produce a large amount of logs (which could indicate that the log silencing isn't working in some cases but it can also be the case that it was like this originally).

@mdedetrich mdedetrich marked this pull request as ready for review February 13, 2023 10:24
@mdedetrich
Copy link
Contributor Author

mdedetrich commented Feb 13, 2023

PR is now ready to review, check original post for details.

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mdedetrich
Copy link
Contributor Author

Will wait for @jrudolph , specifically for comments around AkkaBuild/PekkoBuild.

@jrudolph
Copy link
Contributor

Cool, will have a look now.

Copy link
Contributor

@jrudolph jrudolph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I browsed through the changes with git diff and tried to find significant changes. A few minor comments but mostly lgtm

The only significant to PekkoDependency is the change of the default, isn't it? And that sounds somewhat reasonable for now (it might become a bit inefficient for development if we automatically choose a new nightly every day and invalidate sbt caches on our machines, but if that becomes a problem we could also fix on a certain version for the time being).

@@ -66,7 +66,6 @@ For example, the above screenshot shows an Apache Pekko FJP dispatchers threads,
named "`default-akka.default-dispatcher2,3,4`" going into the blocking state, after having been idle.
It can be observed that the number of new threads increases, "`default-akka.actor.default-dispatcher 18,19,20,...`"
however they go to sleep state immediately, thus wasting the resources.
@java[The same happens to the global @apidoc[ForkJoinPool] when using Java Futures.]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was that deleted intentionally? (Does not matter a lot)

Copy link
Contributor Author

@mdedetrich mdedetrich Feb 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah damn, forgot to mention this. So basically I cannot get this to compile because beforehand due to some shadowing rules it was picking up akka.dispatch.forkjoin.ForkJoinPool and when I tried changing this to a FQCN (i.e. java.util.concurrent.ForkJoinPool) as part of this PR it didn't compile however I noticed that both for current Akka and Pekko before this PR change it doesn't even render on the website so I just removed it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe, just add a Github issue to revisit this?

Copy link
Contributor Author

@mdedetrich mdedetrich Feb 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, can do, will do it after PR merge so I can point the issue to correct part of code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case it is not clear, the @java block would only show if Java is selected in the nav bar.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, indeed, no need to fix it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case it is not clear, the @java block would only show if Java is selected in the nav bar.

Ah I see, but yes I will make an issue on this

//#bindAndHandleSecure
//#bindAndHandlePlain
import akka.http.scaladsl.Http
import org.apache.pekko
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The structure of the imports and how they are referenced into the docs seems to have changed here, was that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if you check the rendered docs with sbt paradox you can see why I need to change the imports statements.

* Copyright (C) 2019-2022 Lightbend Inc. <https://www.lightbend.com>
*/

package akka.http.javadsl.model;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a bunch of those files, the changes seem to be big enough that git does not detect the move any more, I guess not a big problem in practice, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For git diff it seems adding the parameter -M1 works (consider renamed when only 10% of the file are the same, instead of default 50%).

For git blame adding -C helps to get the original commits.

@mdedetrich
Copy link
Contributor Author

mdedetrich commented Feb 14, 2023

The only significant to PekkoDependency is the change of the default, isn't it? And that sounds somewhat reasonable for now (it might become a bit inefficient for development if we automatically choose a new nightly every day and invalidate sbt caches on our machines, but if that becomes a problem we could also fix on a certain version for the time being).

Yes this is correct, I actually want to make an issue on this because my current impressions is this part of the build seems quite over-engineered but lets talk about it in the future issue I am talking about.

@mdedetrich
Copy link
Contributor Author

Okay so I am going to go ahead and merge this, will also make an issue regarding #34 (comment)

@mdedetrich
Copy link
Contributor Author

Issue regarding removed text created at #42

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename package from akka to org.apache.pekko.
3 participants