Upgrade to Akka 2.6.12 #5065
Upgrade to Akka 2.6.12 #5065
Conversation
|
This is failing due to scalafmt error: |
|
Thanks for the contribution. I have a couple of thoughts / experiences with akka 2.6. We eventually undoubtedly need to upgrade this project to akka 2.6.
From my own team's experience with our own services, upgrading to akka 2.6 ended up being quite the headache that was a month's long process of learning little nuances and changes in behavior. I can try to collect all of those things and communicate that knowledge with the community, hopefully that can make the process smooth. The upgrade was valuable with the new features and improvements akka 2.6 offers even with the problems we faced. We need to do this upgrade at some point to keep up with akka, but there is going to need to be more community involvement and discussion on how we want to make this happen rather than just merging the one mr. |
|
Oh also akka 2.6.11 has a critical memory leak in akka streams so it's unusable. |
|
@bdoyle0182 Thanks for the feedback! From our side, we are working on the new feature built with akka typed, which we would like eventually contribute back to this project as well. So right now there are few choices, either a) to downgrade it to akka classic, or b) to upgrade openwhisk to akka 2.6 or c) to keep our own fork. I can tell that the option b) is my favorite and I would be glad to help to move it forward as far as I can. Regarding your comments:
|
|
@bdoyle0182 thanks for the insights you provided regarding the next Akka version. I wasn’t aware of all these details and I’m glad you pointed them out ! To the 3rd point we’re also planning to contribute an add-on to the existing system that improves Web Actions invocation considerably. This PR is a prerequisite for that. 3.a. Is there a reference to the work you mentioned that we could include in this conversation ? 3.b. I’d love to find a way to not block other contributions from the community. The more we iterate on ideas, and the faster we test them out, the faster we’ll learn how to improve the current solution. |
Codecov Report
@@ Coverage Diff @@
## master #5065 +/- ##
==========================================
- Coverage 82.80% 75.17% -7.64%
==========================================
Files 207 211 +4
Lines 10034 10211 +177
Branches 444 435 -9
==========================================
- Hits 8309 7676 -633
- Misses 1725 2535 +810
Continue to review full report at Codecov.
|
| val engine = context.createSSLEngine(host, port) | ||
| engine.setUseClientMode(true) | ||
| // WARNING: this creates an SSL Engine without enabling endpoint identification/verification procedures | ||
| // Disabling host name verification is a very bad idea, please don't unless you have a very good reason to. |
There was a problem hiding this comment.
can you add a comment why it is ok here, given the warning in the comment? ie when is withDisableHostnameVerification set and ok?
There was a problem hiding this comment.
This comment and the implementation is taken from the akka 2.6.x documentation on how to disable host name verification for ssl. This is what openwhisk was doing in couple of places including tests, but having the old SSLConfig class deprecated it had to be re-implemented. So I figured it, I'd rather keep comment to discourage using this approach anywhere in production.
Do you suggest adding more explanation on why is it needed to the code? Sure, I can do that.
There was a problem hiding this comment.
Ah - thanks for the added clarification. I think this was/is needed for testing with self-signed certs.
...on/scala/src/main/scala/org/apache/openwhisk/core/containerpool/logging/SplunkLogStore.scala
Show resolved
Hide resolved
|
@rabbah Thank you for the review! I do have problems with Travis, right now there are couple of tests failing randomly. One in the standalone test suite, one in system tests and one in the unit tests. I'm not sure how common is it for this build, it doesn't look like the reason of the failures in the PR. But if it was stable before then definitely worth investigating more. |
|
@bdoyle0182 @rabbah I have few more questions regarding the upgrade.
|
vrann
force-pushed
the
ow-5060
branch
3 times, most recently
from
5abf9d6 to
8efcab3
Compare
Codecov Report
@@ Coverage Diff @@
## master #5065 +/- ##
===========================================
+ Coverage 41.02% 75.97% +34.95%
===========================================
Files 226 226
Lines 11470 11463 -7
Branches 491 496 +5
===========================================
+ Hits 4705 8709 +4004
+ Misses 6765 2754 -4011
Continue to review full report at Codecov.
|
|
vrann
force-pushed
the
ow-5060
branch
3 times, most recently
from
e2584e5 to
b611036
Compare
- removed usage implicit custom ActorMaterializer - upgraded to Akka Http 10.2.3 - changed the way how Https set up when host name verification is disabled - changed RestartSink.withBackoff to accept RestartSettings - changed akka.actor.FSM.setTimer => akka.actor.FSM.startTimerAtFixedRate - changed akka.actor.Scheduler.schedule => akka.actor.Scheduler.scheduleAtFixedRate - updated to the new signature of Source.actorRef - replaced deprecated HttpResponse.copy - replaced Gzip with Coders.Gzip - fixed the scalafmt in tests - removed the implicit Materializer shutdown considering its lifecycle is managed by akka - increased the ansible wait time - switched to classic networking - disabled JFR - merging with New Scheduler - increased wait time for FSM due to intermittent failures in build
|
@bdoyle0182 Are you suggesting a version of this PR with akka version 2.6.2 instead and a subsequent PR that bumps the version to 2.6.12? |
|
@rabbah I think the suggestion is dependent on the way OpenWhisk is being operated / upgraded. I'm worried that if we wait longer, we're gonna have more conflicts to fix in this PR, depending on changes in |
|
Thanks Dragos - I agree. I think one approach for operators is to change the version in the operator's fork or we can create a branch that downgrade the akka version. I don't expect that we'll cut a release with the transition version and then another with the new version. I reviewed the PR again since it was a while already since I last looked at these changes. I don't have any reservations. |
|
@rabbah thanks for reviewing this PR again ! @bdoyle0182 I'm merging this with the assumption that if you need to transition through intermediate Akka versions you can branch this, change the Akka version, rebuild, deploy, and repeat until you reach the Akka version in this PR. Please keep us posted on how this goes and if you encounter any issues along the way. |
Implement #5060
For any new features which uses Akka actors it would makes sense to start using Behavior-based construction of actors with the Akka Typed. This is the way to construct actors encouraged by Akka for any new functionality. In order to use Akka Typed, it should be supported by the Akka version used in OpenWhisk. At the same time no changes needed to the Akka Classic actors. Upgrading to the newest Akka version would allow to use all latest features of the Akka, while keeping it compatible with almost everything of the Akka 2.5.x that is been used in OpenWhisk so far.
Description
Related issue and scope
My changes affect the following components
Types of changes
Checklist: