You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by shijinkui <gi...@git.apache.org> on 2016/09/23 09:51:17 UTC
[GitHub] flink pull request #2541: [FLINK-4669] scala api createLocalEnvironment() fu...
GitHub user shijinkui opened a pull request:
https://github.com/apache/flink/pull/2541
[FLINK-4669] scala api createLocalEnvironment() function add default Configuration parameter
1. add Configuration as createLocalEnvironment second default paramter
2. fix the mistake scaladoc usage of readFile function
3. delete the brackets if method have no parameter. Because methods that follow JavaBean naming contract for accessors are expected to have no side effects. The recommended convention is to use a parameterless method whenever there are no parameters and the method have no side effect.
- [ ] General
- The pull request references the related JIRA issue ("[FLINK-4669] scala api createLocalEnvironment() function add default Configuration parameter")
- The pull request addresses only one issue
- Each commit in the PR has a meaningful commit message (including the JIRA id)
- [ ] Documentation
- Documentation has been added for new functionality
- Old documentation affected by the pull request has been updated
- JavaDoc for public methods has been added
- [ ] Tests & Build
- Functionality added by the pull request is covered by tests
- `mvn clean verify` has been executed successfully locally or a Travis build has passed
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shijinkui/flink create_local_env_with_conf
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2541.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 #2541
----
commit f79ec4c3cb20d7be496e864aac7c5619199f96a4
Author: shijinkui <sh...@huawei.com>
Date: 2016-09-23T09:46:08Z
[FLINK-4669] scala api createLocalEnvironment() function add default Configuration parameter
1.add Configuration as createLocalEnvironment second default paramter
2.fix the mistake scaladoc usage of readFile function
3.Methods that follow JavaBean naming contract for accessors are expected to have no side effects. The recommended convention is to use a parameterless method whenever there are no parameters and the method have no side effect.
----
---
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 #2541: [FLINK-4669] scala api createLocalEnvironment() fu...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/2541#discussion_r80984450
--- Diff: flink-streaming-scala/src/main/scala/org/apache/flink/streaming/api/scala/StreamExecutionEnvironment.scala ---
@@ -124,7 +125,7 @@ class StreamExecutionEnvironment(javaEnv: JavaEnv) {
* Gets the checkpoint config, which defines values like checkpoint interval, delay between
* checkpoints, etc.
*/
- def getCheckpointConfig = javaEnv.getCheckpointConfig()
--- End diff --
That is fine, IDEA may have different recommendations, Flink can define its own guidelines.
I (and others) don't agree with everything that the Scala folks recommend.
And even in that description, it is phrased as a possibility to bridge the gap between Scala's and Java's features.
---
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 #2541: [FLINK-4669] scala api createLocalEnvironment() function ...
Posted by shijinkui <gi...@git.apache.org>.
Github user shijinkui commented on the issue:
https://github.com/apache/flink/pull/2541
@StephanEwen have any improvement needed?
---
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 #2541: [FLINK-4669] scala api createLocalEnvironment() function ...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2541
Code looks good, but the build detected a break in binary compatibility. Have a look at the Travis logs.
Changing the `createLocalEnvironment` to use an additional parameter with default value broke the compatibility. The fix would be to have an extra method (and no default parameter value), not change the existing method.
---
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 #2541: [FLINK-4669] scala api createLocalEnvironment() function ...
Posted by shijinkui <gi...@git.apache.org>.
Github user shijinkui commented on the issue:
https://github.com/apache/flink/pull/2541
@StephanEwen I\u2018ve a new function name `createCustomLocalEnv`. Sorry for late ack.
---
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 #2541: [FLINK-4669] scala api createLocalEnvironment() function ...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2541
Sorry for asking for another iteration, but here are some comments:
- The other Java code uses the primitive `int` everywhere, rather than the boxed integer. Let's follow that style.
- I would omit the `public static ExecutionEnvironment createCustomLocalEnv(Integer parallelism, Configuration config)` variant. The benefit is very small compared to the `public static ExecutionEnvironment createLocalEnvironment(Configuration config)` method, but it blows up the API into too many variants.
- Similarly, I would only have the `public static ExecutionEnvironment createLocalEnvWithWebUI (int port, Configuration config)` and drop the variant with the additional `parallelism` parameter.
In general, I think it is good to not have too many method options, but only those that add significant functionality. For the method that starts the WebUI, that is the case, for the method with the additional parallelism, probably not.
---
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 #2541: [FLINK-4669] scala api createLocalEnvironment() function ...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2541
Thanks, picking this pull request up and merging it...
---
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 #2541: [FLINK-4669] scala api createLocalEnvironment() fu...
Posted by shijinkui <gi...@git.apache.org>.
Github user shijinkui commented on a diff in the pull request:
https://github.com/apache/flink/pull/2541#discussion_r80963928
--- Diff: flink-streaming-scala/src/main/scala/org/apache/flink/streaming/api/scala/StreamExecutionEnvironment.scala ---
@@ -124,7 +125,7 @@ class StreamExecutionEnvironment(javaEnv: JavaEnv) {
* Gets the checkpoint config, which defines values like checkpoint interval, delay between
* checkpoints, etc.
*/
- def getCheckpointConfig = javaEnv.getCheckpointConfig()
--- End diff --
IDEA ful-tip:
Methods that follow JavaBean naming contract for accessors are expected to have no side effects. The recommended convention is to use a parameterless method whenever there are no parameters and the method have no side effect. This convention supports the uniform access principle, which says that client code should not be affected by a decision to implement an attribute as a field or method. The problem is that Java does not implement the uniform access principle. To bridge that gap, Scala allows you to leave off the empty parentheses on an invocation of function that takes no arguments. * Refer to Programming in Scala, 10.3 Defining parameterless methods
---
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 #2541: [FLINK-4669] scala api createLocalEnvironment() function ...
Posted by shijinkui <gi...@git.apache.org>.
Github user shijinkui commented on the issue:
https://github.com/apache/flink/pull/2541
hi, @StephanEwen
follow your exact suggest, have add the functions at four place.
---
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 #2541: [FLINK-4669] scala api createLocalEnvironment() function ...
Posted by shijinkui <gi...@git.apache.org>.
Github user shijinkui commented on the issue:
https://github.com/apache/flink/pull/2541
@StephanEwen review it again please.
---
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 #2541: [FLINK-4669] scala api createLocalEnvironment() fu...
Posted by shijinkui <gi...@git.apache.org>.
Github user shijinkui commented on a diff in the pull request:
https://github.com/apache/flink/pull/2541#discussion_r81031381
--- Diff: flink-streaming-scala/src/main/scala/org/apache/flink/streaming/api/scala/StreamExecutionEnvironment.scala ---
@@ -124,7 +125,7 @@ class StreamExecutionEnvironment(javaEnv: JavaEnv) {
* Gets the checkpoint config, which defines values like checkpoint interval, delay between
* checkpoints, etc.
*/
- def getCheckpointConfig = javaEnv.getCheckpointConfig()
--- End diff --
That's fine.
---
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 #2541: [FLINK-4669] scala api createLocalEnvironment() fu...
Posted by shijinkui <gi...@git.apache.org>.
Github user shijinkui commented on a diff in the pull request:
https://github.com/apache/flink/pull/2541#discussion_r80966754
--- Diff: flink-streaming-scala/src/main/scala/org/apache/flink/streaming/api/scala/StreamExecutionEnvironment.scala ---
@@ -124,7 +125,7 @@ class StreamExecutionEnvironment(javaEnv: JavaEnv) {
* Gets the checkpoint config, which defines values like checkpoint interval, delay between
* checkpoints, etc.
*/
- def getCheckpointConfig = javaEnv.getCheckpointConfig()
--- End diff --
http://stackoverflow.com/questions/8124055/scala-tostring-parenthesize-or-not http://stackoverflow.com/questions/12334936/why-does-scala-need-parameterless-in-addition-to-zero-parameter-methods
---
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 #2541: [FLINK-4669] scala api createLocalEnvironment() fu...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/2541#discussion_r80730795
--- Diff: flink-streaming-scala/src/main/scala/org/apache/flink/streaming/api/scala/StreamExecutionEnvironment.scala ---
@@ -124,7 +125,7 @@ class StreamExecutionEnvironment(javaEnv: JavaEnv) {
* Gets the checkpoint config, which defines values like checkpoint interval, delay between
* checkpoints, etc.
*/
- def getCheckpointConfig = javaEnv.getCheckpointConfig()
--- End diff --
Calling Java methods with parenthesis or without actually somewhat subject to debate.
For Scala methods, the writer decides whether it is a method with or without parenthesis.
For Java, the caller can only "guess" what the method does (is returning a mutable field considered a side effect?), so in most places we call Java methods as Java methods.
---
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 #2541: [FLINK-4669] scala api createLocalEnvironment() function ...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2541
Okay, this has binary compatibility now.
Few things I would still suggest to do:
- There is still a bug that the `parallelism` and `port` parameter are mixed up.
- It still does not use the DEFAULT_LOCAL_PARALLELISM.
- The same methods should exist in all environments (batch / streaming / Java / Scala)
---
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 #2541: [FLINK-4669] scala api createLocalEnvironment() function ...
Posted by shijinkui <gi...@git.apache.org>.
Github user shijinkui commented on the issue:
https://github.com/apache/flink/pull/2541
> In general, I think it is good to not have too many method options, but only those that add significant functionality. For the method that starts the WebUI, that is the case, for the method with the additional parallelism, probably not.
Function having least parameters is the best style. If have no enough parameter for user, the function's ability is limited. That's why I prefer Scala, as it has default parameter value, keep the both.
> The other Java code uses the primitive int everywhere, rather than the boxed integer. Let's follow that style.
Good idea. In the latest two years, I only wirte scala and give up Java entirely so that not sensitive to Java.
---
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 #2541: [FLINK-4669] scala api createLocalEnvironment() fu...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/2541
---
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.
---