[GitHub] flink pull request: [FLINK-1589] Add option to pass configuration ...
Github user rmetzger commented on the pull request: https://github.com/apache/flink/pull/427#issuecomment-88794279 I'm merging this 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-1589] Add option to pass configuration ...
Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/427 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-1589] Add option to pass configuration ...
Github user rmetzger commented on the pull request: https://github.com/apache/flink/pull/427#issuecomment-88453493 If there are no objections, I'm going to merge the PR in the next 24 hours. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-1589] Add option to pass configuration ...
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/427#discussion_r27564491
--- Diff:
flink-clients/src/main/java/org/apache/flink/client/LocalExecutor.java ---
@@ -103,6 +110,9 @@ public void start() throws Exception {
// create the embedded runtime
Configuration configuration =
getConfiguration(this);
+ if(this.configuration != null) {
+
configuration.addAll(this.configuration);
+ }
--- End diff --
Wouldn't it be better to move the null check to the constructor and create
the default `Configuration` there via a call to `getConfiguration`?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1589] Add option to pass configuration ...
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/427#discussion_r27564493
--- Diff:
flink-clients/src/main/java/org/apache/flink/client/LocalExecutor.java ---
@@ -70,6 +72,11 @@ public LocalExecutor() {
}
}
+ public LocalExecutor(Configuration conf) {
+ super();
+ this.configuration = conf;
+ }
--- End diff --
Should `super()` not be actually `this()`?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1589] Add option to pass configuration ...
Github user uce commented on a diff in the pull request:
https://github.com/apache/flink/pull/427#discussion_r27566199
--- Diff:
flink-clients/src/main/java/org/apache/flink/client/LocalExecutor.java ---
@@ -103,6 +110,9 @@ public void start() throws Exception {
// create the embedded runtime
Configuration configuration =
getConfiguration(this);
+ if(this.configuration != null) {
+
configuration.addAll(this.configuration);
+ }
--- End diff --
+1, I would rename getConfiguration to createConfiguration as well
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1589] Add option to pass configuration ...
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/427#discussion_r27564611
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/LocalEnvironment.java ---
@@ -32,6 +33,7 @@
* machine.
*/
public class LocalEnvironment extends ExecutionEnvironment {
+ private Configuration configuration = null;
--- End diff --
The `= null` initializations are unnecessary.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1589] Add option to pass configuration ...
Github user uce commented on a diff in the pull request:
https://github.com/apache/flink/pull/427#discussion_r27566259
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/ExecutionEnvironment.java ---
@@ -1058,6 +1059,20 @@ public static LocalEnvironment
createLocalEnvironment(int parallelism) {
lee.setParallelism(parallelism);
return lee;
}
+
+ /**
+* Creates a {@link LocalEnvironment}. The local execution environment
will run the program in a
+* multi-threaded fashion in the same JVM as the environment was
created in. It will use the
+* degree of parallelism specified in the parameter.
--- End diff --
degree of parllelism has been renamed recently
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1589] Add option to pass configuration ...
Github user StephanEwen commented on the pull request: https://github.com/apache/flink/pull/427#issuecomment-88462555 Looks good. It is possible to add a more Unit-Test style test, rather than an Integration test case (starting a full mini cluster) ? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-1589] Add option to pass configuration ...
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/427#discussion_r27569381
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/ExecutionEnvironment.java ---
@@ -1058,6 +1059,20 @@ public static LocalEnvironment
createLocalEnvironment(int parallelism) {
lee.setParallelism(parallelism);
return lee;
}
+
+ /**
+* Creates a {@link LocalEnvironment}. The local execution environment
will run the program in a
+* multi-threaded fashion in the same JVM as the environment was
created in. It will use the
+* degree of parallelism specified in the parameter.
--- End diff --
+1 please change it to parallelism :)
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1589] Add option to pass configuration ...
Github user uce commented on the pull request: https://github.com/apache/flink/pull/427#issuecomment-88473959 Looks good. - Can you add missing `@Override` tags to `executePlan` and `getOptimizerPlanAsJSON` while editing the file? - If you only want to ensure that the respective Configuration values are set, we could indeed just get the configuration object and check that the respective values keys are set as expected. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-1589] Add option to pass configuration ...
Github user rmetzger commented on the pull request: https://github.com/apache/flink/pull/427#issuecomment-86979441 I've updated the PR. It is now ready for review 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-1589] Add option to pass configuration ...
Github user rmetzger commented on the pull request: https://github.com/apache/flink/pull/427#issuecomment-75542199 @tillrohrmann: Thank you for the good feedback! I'll continue working on this once https://github.com/apache/flink/pull/410 is merged to master. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-1589] Add option to pass configuration ...
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/427#discussion_r25152294
--- Diff:
flink-tests/src/test/java/org/apache/flink/test/javaApiOperators/ExecutionEnvironmentITCase.java
---
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * License); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.test.javaApiOperators;
+
+import org.apache.flink.api.java.DataSet;
+import org.apache.flink.api.java.ExecutionEnvironment;
+import org.apache.flink.configuration.ConfigConstants;
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.test.javaApiOperators.util.CollectionDataSets;
+import org.apache.flink.test.util.MultipleProgramsTestBase;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.util.ArrayList;
+import java.util.Collection;
+
+
+@RunWith(Parameterized.class)
+public class ExecutionEnvironmentITCase extends MultipleProgramsTestBase {
+
+
+ public ExecutionEnvironmentITCase(ExecutionMode mode) {
+ super(mode);
+ }
+
+ @Parameterized.Parameters(name = Execution mode = {0})
+ public static CollectionExecutionMode[] executionModes(){
+ CollectionExecutionMode[] c = new
ArrayListExecutionMode[](1);
+ c.add(new ExecutionMode[] {ExecutionMode.CLUSTER});
+ return c;
+ }
+
+
+ @Test
+ public void testLocalEnvironmentWithConfig() throws Exception {
+ IllegalArgumentException e = null;
+ try {
+ Configuration conf = new Configuration();
+
conf.setBoolean(ConfigConstants.FILESYSTEM_DEFAULT_OVERWRITE_KEY, true);
+
conf.setString(ConfigConstants.TASK_MANAGER_TMP_DIR_KEY,
/tmp/thelikelyhoodthatthisdirectoryexisitsisreallylow);
--- End diff --
To be on the safe side, we used in other tests a temp directory (specially
created) where we removed the write permissions for the user (and the in
finally re-grant them in order to cleanly remove the directory)
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1589] Add option to pass configuration ...
GitHub user rmetzger opened a pull request: https://github.com/apache/flink/pull/427 [FLINK-1589] Add option to pass configuration to LocalExecutor Please review the changes. I'll add a testcase and update the documentation later today. You can merge this pull request into a Git repository by running: $ git pull https://github.com/rmetzger/flink flink1589 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/427.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 #427 commit b75b4c285f4810faa5d02d638b61dc7b8e125c8d Author: Robert Metzger rmetz...@apache.org Date: 2015-02-20T11:40:41Z [FLINK-1589] Add option to pass configuration to LocalExecutor --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-1589] Add option to pass configuration ...
Github user rmetzger commented on the pull request: https://github.com/apache/flink/pull/427#issuecomment-75244751 I've added documentation and tests to the change. Lets see if travis gives us a green light. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-1589] Add option to pass configuration ...
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/427#discussion_r25080967
--- Diff:
flink-tests/src/test/java/org/apache/flink/test/javaApiOperators/ExecutionEnvironmentITCase.java
---
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * License); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.test.javaApiOperators;
+
+import org.apache.flink.api.java.DataSet;
+import org.apache.flink.api.java.ExecutionEnvironment;
+import org.apache.flink.configuration.ConfigConstants;
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.test.javaApiOperators.util.CollectionDataSets;
+import org.apache.flink.test.util.MultipleProgramsTestBase;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.util.ArrayList;
+import java.util.Collection;
+
+
+@RunWith(Parameterized.class)
+public class ExecutionEnvironmentITCase extends MultipleProgramsTestBase {
+
+
+ public ExecutionEnvironmentITCase(ExecutionMode mode) {
+ super(mode);
+ }
+
+ @Parameterized.Parameters(name = Execution mode = {0})
+ public static CollectionExecutionMode[] executionModes(){
+ CollectionExecutionMode[] c = new
ArrayListExecutionMode[](1);
+ c.add(new ExecutionMode[] {ExecutionMode.CLUSTER});
+ return c;
+ }
+
+
+ @Test
+ public void testLocalEnvironmentWithConfig() throws Exception {
+ IllegalArgumentException e = null;
+ try {
+ Configuration conf = new Configuration();
+
conf.setBoolean(ConfigConstants.FILESYSTEM_DEFAULT_OVERWRITE_KEY, true);
+
conf.setString(ConfigConstants.TASK_MANAGER_TMP_DIR_KEY,
/tmp/thelikelyhoodthatthisdirectoryexisitsisreallylow);
--- End diff --
Are you sure?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1589] Add option to pass configuration ...
Github user tillrohrmann commented on the pull request: https://github.com/apache/flink/pull/427#issuecomment-75271706 I think we should rework the test case to check that the configuration is properly passed to the system. Right now the exception is thrown in ```ds.writeAsText(null)``` because we pass ```null```. I'd propose something like @StephanEwen did in the PR #410. We set the number of slots in the configuration and the job to ```PARALLELISM_AUTO_MAX```. With the special input format which produces only a single element per split, we can count the number of parallel tasks, given that every task receives only one input split. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
