-
Notifications
You must be signed in to change notification settings - Fork 13k
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
[FLINK-7810] Switch from custom Flakka to Akka 2.4.x #4807
Conversation
wonderful! +1 |
We can do this now that we dropped support for Scala 2.10.
e64135f
to
a3e1ef6
Compare
docs/start/building.md
Outdated
|
||
Newer versions may be compatible, depending on breaking changes in the language features used by Flink, and the availability of Flink's dependencies in those Scala versions. The dependencies written in Scala include for example *Kafka*, *Akka*, *Scalatest*, and *scopt*. | ||
|
||
**By default, Flink is built with the Scala 2.11**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would drop the, i.e.,
By default, Flink is built with Scala 2.11.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixing
|
||
To build against custom Scala versions, you need to define new custom build profile that will override *scala.version* and *scala.binary.version* values. | ||
|
||
Flink is developed against Scala *2.11* and tested additionally against Scala *2.10*. These two versions are known to be compatible. Earlier versions (like Scala *2.9*) are *not* compatible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read this paragraph to imply that Flink is still compatible with 2.10, but is that accurate given the use of Akka 2.4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I don't explicitly say what we're compatible with now. In fact we only support 2.11 now, supporting 2.12 is a bit harder than I though.
Thanks for doing this, we need Akka 2.4 badly for improved SSL support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, with minor comments...
.travis.yml
Outdated
@@ -61,31 +61,6 @@ matrix: | |||
- TEST="misc" | |||
- PROFILE="-Dhadoop.version=2.8.0" | |||
- CACHE_NAME=JDK8_H280_M | |||
- jdk: "openjdk8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about retaining these Travis profiles for now for the Hadoop and JDK versions?
Just bump the Scala dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, of course. Undropping ...
@@ -110,8 +110,8 @@ under the License. | |||
</dependency> | |||
|
|||
<dependency> | |||
<groupId>com.data-artisans</groupId> | |||
<artifactId>flakka-testkit_${scala.binary.version}</artifactId> | |||
<groupId>com.typesafe.akka</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not find any use of akka testkit in the queryable state client (and there should not be, design wise).
Was this just copy/paste forwarded and can be dropped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a test in there that was using Akka but I think that's not needed anymore. Will remove.
On a side node, currently, the queryable-state-client module contains both the client and server implementations (that's why there was the akka test) while flunk-runtime only has interfaces. Fully tweezing the client and server apart is still in progress.
@@ -202,8 +202,6 @@ protected void run() { | |||
assertTrue(ioManager.isProperlyShutDown()); | |||
assertTrue(memManager.isShutdown()); | |||
} finally { | |||
TestingUtils.stopActorsGracefully(Arrays.asList(jobManager, taskManager)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this now happening automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the test succeeds, they will have been shutdown (the previous asserts also assert (hehe) this.
It seems the previous Akka version didn't mind another shutdown attempt but 2.4 fails with an exception when you try to shutdown after already being shut down.
I think this is good now, +1 |
We can do this now that we dropped support for Scala 2.10. This closes apache#4807
What is the purpose of the change
Drop support for Scala 2.10 and then update to a newer Akka version. Before, we were forced to stay on our custom Flakka 2.3.x version with back ports because newer Akka does not support Scala 2.10.