You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by tillrohrmann <gi...@git.apache.org> on 2016/08/31 23:53:25 UTC
[GitHub] flink pull request #2450: [Flink-4458] Replace ForkableFlinkMiniCluster by L...
GitHub user tillrohrmann opened a pull request:
https://github.com/apache/flink/pull/2450
[Flink-4458] Replace ForkableFlinkMiniCluster by LocalFlinkMiniCluster
This PR is based on #2449. Thus, only the commits 5a2c70f and e07ad44 are relevant.
The PR refactors the `LocalFlinkMiniCluster` and the `TestingCluster` so that they can replace the `ForkableFlinkMiniCluster`. The `LocalFlinkMiniCluster` now starts all distributed components under a random port. This is possible with #2449.
The difference of the `TestingCluster` compared to the `LocalFlinkMiniCluster` is that it starts the testing components (e.g. `TestingJobManager`, `TestingTaskManager`, etc.) instead of the plain ones. It also allows to start the `JobManager` in a synchronous mode. Furthermore, it offers some operations like restarting the leading `JobManager` under the same port.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tillrohrmann/flink FLINK-4458
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2450.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2450
----
commit 96b1772d4581365eeec6614827b36f76a83ba5d0
Author: Till Rohrmann <tr...@apache.org>
Date: 2016-08-31T07:33:46Z
[FLINK-4455] [FLINK-4424] [networkenv] Make NetworkEnvironment independent of ActorGateway and JobManager association
Makes the NetworkEnvironment independent of the JobManager association. This means that the
NetworkEnvironment and with it the ConnectionManager is started before the TaskManager actor
is executed. Furthermore, the ConnectionManager keeps running even in case of a JobManager
disassocation. In the wake of the remodelling this behaviour, the PartitionStateChecker and
the ResultPartitionConsumableNotifier which depend on the JobManager association were moved
out of the NetworkEnvironment. They are now contained in the SlotEnvironment which will be
set up when the TaskManager connects to a JobManager. The SlotEnvironment contains all
information related to the associated JobManager. Since all slots are implicitly associated
with the JobManager which is the leader, we only create one SlotEnvironment which is shared
by all Tasks.
Introduce SlotEnvironment to accommodate the PartitionStateChecker and ResultPartitionConsumableNotifier
Remove the PartitionStateChecker and the ResultPartitionConsumableNotifier from the
NetworkEnvironment. Start the NetworkEnvironment when the TaskManager components are
created. Keep the NetworkEnvironment running also when the JobManager is disassociated.
commit 5a2c70fccfc480d777788b2697af3cd0eaf149a2
Author: Till Rohrmann <tr...@apache.org>
Date: 2016-08-31T15:58:09Z
[FLINK-4458] Refactored LocalFlinkMiniCluster and TestingCluster
commit e07ad443421edba93bcb3ac7831909b6fd012a51
Author: Till Rohrmann <tr...@apache.org>
Date: 2016-08-31T16:25:07Z
[FLINK-4458] Replace ForkableFlinkMiniCluster by LocalFlinkMiniCluster
Add RequestNumActiveConnections and RequestBroadcastVariablesWithReferences to TaskManager
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2450: [FLINK-4458] Replace ForkableFlinkMiniCluster by L...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/2450#discussion_r77180730
--- Diff: flink-runtime/src/main/scala/org/apache/flink/runtime/minicluster/FlinkMiniCluster.scala ---
@@ -69,7 +69,7 @@ abstract class FlinkMiniCluster(
ConfigConstants.JOB_MANAGER_IPC_ADDRESS_KEY,
InetAddress.getByName("localhost").getHostAddress())
- val configuration = generateConfiguration(userConfiguration)
+ protected val _configuration = generateConfiguration(userConfiguration)
--- End diff --
Why the underscore here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2450: [FLINK-4458] Replace ForkableFlinkMiniCluster by LocalFli...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2450
Quick question: The `ForkableFlinkMiniCluster` was something semi-public, in the sense that it was part of the `flink-test-utils` project and used by quite a few users. So this would be a breaking change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2450: [FLINK-4458] Replace ForkableFlinkMiniCluster by L...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/2450#discussion_r77212855
--- Diff: flink-runtime/src/main/scala/org/apache/flink/runtime/minicluster/FlinkMiniCluster.scala ---
@@ -69,7 +69,7 @@ abstract class FlinkMiniCluster(
ConfigConstants.JOB_MANAGER_IPC_ADDRESS_KEY,
InetAddress.getByName("localhost").getHostAddress())
- val configuration = generateConfiguration(userConfiguration)
+ protected val _configuration = generateConfiguration(userConfiguration)
--- End diff --
It's actually the original configuration which will usually have the job manager ipc port set to 0. Thus, in order to return a configuration which points to the current leader, we have to generate a new configuration via `configuration` which inserts the selected port. I'll rename the variable to `originalConfiguration`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2450: [FLINK-4458] Replace ForkableFlinkMiniCluster by LocalFli...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/2450
Will rebase on the latest master and #2449 to run the travis tests again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2450: [FLINK-4458] Replace ForkableFlinkMiniCluster by LocalFli...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/2450
Yes that is true. However, the `flink-test-utils` have not been guaranteed to be stable if I'm not mistaken. The changes one has to apply is to replace the `ForkableFlinkMiniCluster` with the `LocalFlinkMiniCluster`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2450: [FLINK-4458] Replace ForkableFlinkMiniCluster by LocalFli...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/2450
I've re-introduced the `ForkableFlinkMiniCluster`. However, this time it only starts the normal distributed component classes (e.g. `JobManager` instead of `TestingJobManager`, etc.). This implies that the testing messages are no longer supported when using the `ForkableFlinkMiniCluster`.
I think the user should not meddle with the testing components in the first place. Therefore, it seems to be fair to offer the user a mini cluster to run his tests but not one which instantiates testing components. The recommended cluster is the `LocalFlinkMiniCluster` from now on. However, in order to not break all existing test code, we still maintain the `ForkableFlinkMiniCluster`.
Not having to start the testing classes allows to move them back into the test scope of `flink-runtime`, since they are no longer required by the `flink-test-utils` module.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2450: [FLINK-4458] Replace ForkableFlinkMiniCluster by LocalFli...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/2450
Thanks for the review @StephanEwen.
I agree that we've different style preferences concerning method parameters. I also agree that the one parameter per line is a little bit more verbose and requires more vertical scrolling. The best approach would probably be to reduce the number of method parameters by grouping them.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2450: [FLINK-4458] Replace ForkableFlinkMiniCluster by LocalFli...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/2450
I will re-introduce the `ForkableFlinkMiniCluster` which is simply extending the `LocalFlinkMiniCluster`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2450: [FLINK-4458] Replace ForkableFlinkMiniCluster by LocalFli...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/2450
Rebasing on the latest master and letting Travis run. If it gives green light, I will merge the PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2450: [FLINK-4458] Replace ForkableFlinkMiniCluster by LocalFli...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/2450
Builds are passing on Travis (own repository). Would like to merge this PR if there are no objections.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2450: [FLINK-4458] Replace ForkableFlinkMiniCluster by LocalFli...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2450
I think these changes are good.
As a very personal and biased statement: I think the code overdoes it a bit with the "every parameter on a new line" policy. We have beautiful wide screens and I get sore fingers from scrolling up and down again to figure out enough context of a code passage ;-)
Also, the style breaks the "visual" separation between different parts of statement, like "members of a return tuple" vs. "function parameters", or "clauses of an if statement" and "body of the if statement". I know the idea of that pattern was to improve code readability, but I am getting the feeling this is approaching a "verschlimmbesserung" (an improvement that makes things worse).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2450: [FLINK-4458] Replace ForkableFlinkMiniCluster by L...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/2450
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---