You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by mxm <gi...@git.apache.org> on 2015/03/16 18:29:42 UTC
[GitHub] flink pull request: [FLINK-1679] rename degree of parallelism to p...
GitHub user mxm opened a pull request:
https://github.com/apache/flink/pull/488
[FLINK-1679] rename degree of parallelism to parallelism & extend documentation about parallelism
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mxm/flink parallelism
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/488.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 #488
----
commit 9acf5057820ef2e9feece3da216cbd40dcb4cfd9
Author: Maximilian Michels <mx...@apache.org>
Date: 2015-03-16T15:52:21Z
[FLINK-1679] rename degree of parallelism to parallelism
* [Dd]egree[ -]of[ -]parallelism -> [pP]arallelism
* DOP -> [pP]arallelism
* paraDegree -> parallelism
* DEGREE_OF_PARALLELISM -> PARALLELISM
* parallelization.degree.default -> parallelism.default
commit b136659524ddcac9bf47b229b8bf0fff29bd171c
Author: Maximilian Michels <mx...@apache.org>
Date: 2015-03-16T17:01:33Z
[FLINK-1679] extend faq and programming guide to clarify 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1679] rename degree of parallelism to p...
Posted by mxm <gi...@git.apache.org>.
GitHub user mxm reopened a pull request:
https://github.com/apache/flink/pull/488
[FLINK-1679] rename degree of parallelism to parallelism & extend documentation about parallelism
https://issues.apache.org/jira/browse/FLINK-1679
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mxm/flink parallelism
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/488.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 #488
----
commit 17097fdf51f41445bad6da2186868185a6bf947b
Author: Maximilian Michels <mx...@apache.org>
Date: 2015-03-18T09:44:42Z
[FLINK-1679] deprecate API methods to set the parallelism
commit f6ba8c07cc9a153b1ac1e213f9749155c42ae3c3
Author: Maximilian Michels <mx...@apache.org>
Date: 2015-03-18T09:44:43Z
[FLINK-1679] use a consistent name for parallelism
* rename occurrences of degree of parallelism to parallelism
* [Dd]egree[ -]of[ -]parallelism -> [pP]arallelism
* (DOP|dop) -> [pP]arallelism
* paraDegree -> parallelism
* degree-of-parallelism -> parallelism
* DEGREE_OF_PARALLELISM -> PARALLELISM
commit 658bb1166aa907677e06cf011e5a0fdaf58ab15f
Author: Maximilian Michels <mx...@apache.org>
Date: 2015-03-18T09:44:44Z
[FLINK-1679] deprecate old parallelism config entry
old config parameter can still be used
OLD
parallelization.degree.default
NEW
parallelism.default
commit 412ac54df0fde12666afbc1414df5fd919ba1607
Author: Maximilian Michels <mx...@apache.org>
Date: 2015-03-18T09:44:45Z
[FLINK-1679] extend faq and programming guide to clarify 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1679] rename degree of parallelism to p...
Posted by uce <gi...@git.apache.org>.
Github user uce commented on the pull request:
https://github.com/apache/flink/pull/488#issuecomment-82212002
Very nice and exhaustive change.
One question though: do we want to just rename configuration parameters and methods like that or should we keep the old config keys (parallelization.degree.default) and the old config methods (setDegreeOfParallelism) as well and deprecate them first?
I think I am in favour of option 2.
---
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: [FLINK-1679] rename degree of parallelism to p...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/488#issuecomment-84058237
Any other opinions on the API breaking nature of these changes?
---
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: [FLINK-1679] rename degree of parallelism to p...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/488#issuecomment-82219329
I think this is very API breaking - in my opinion, we should keep the old methods where it breaks the API, or at least keep them for one more version (deprecated).
That does not prevent us from merging the fixes to comments and docs.
---
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: [FLINK-1679] rename degree of parallelism to p...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/488#issuecomment-84061320
@StephanEwen That's exactly what I did.
---
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: [FLINK-1679] rename degree of parallelism to p...
Posted by mxm <gi...@git.apache.org>.
Github user mxm closed the pull request at:
https://github.com/apache/flink/pull/488
---
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: [FLINK-1679] rename degree of parallelism to p...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/488#issuecomment-84875935
Merged in master with 126f9f799071688fe80955a7e7cfa991f53c95af
---
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: [FLINK-1679] rename degree of parallelism to p...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/488#issuecomment-83095571
Any objections against merging 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1679] rename degree of parallelism to p...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/488#issuecomment-82392874
I've updated the pull request to include deprecation of the setters/getters and the config parameter.
---
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: [FLINK-1679] rename degree of parallelism to p...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/488#issuecomment-82825369
Thank you for the huge change! Our users will appreciate the more concise naming and improved documentation.
+1 to merge
---
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: [FLINK-1679] rename degree of parallelism to p...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/488#issuecomment-83433378
Let me explain how I made out the relevant API facing methods to deprecate. Since I changed all calls to set the parallelism to `getParallelism` or `setParallelism` I did not have to change classes which already used these methods. To find the remaining which used `setDegreeOfParalleism` or `getDegreeOfParalleism` method, I did a regexp search in the Java and Scala classes:
Java: `public .* (get|set)DegreeOfParallelism`
`ExecutionConfig`
`Operator`
`CollectionEnvironment`
`ExecutionEnvironment`
`StreamExecutionEnvironment`
`SpargelIteration`
Scala: `def (get|set)DegreeOfParallelism`
`ExecutionEnvironment`
`StreamExecutionEnvironment`
In these classes I deprecated the methods, and made them call the new methods `getParallelism` and `setParallelism`. That's how I verified this pull request is not API breaking. For you inspection, I've created a separate commit with these changes. The deprecated methods are also covered in the documentation about how to change the parallelism: http://ci.apache.org/projects/flink/flink-docs-master/programming_guide.html#parallel-execution
---
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: [FLINK-1679] rename degree of parallelism to p...
Posted by mxm <gi...@git.apache.org>.
Github user mxm closed the pull request at:
https://github.com/apache/flink/pull/488
---
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: [FLINK-1679] rename degree of parallelism to p...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/488#issuecomment-84059897
I think all changes that are in `flink-core`, `flink-java`, `flink-scala`, `flink-streaming`, `flink-spargel` are critical. For all those changes, there should be an additional method and the current one should be deprecated.
Other changes (Runtime, Optimizer, tests) can be made without any problem.
*NOTE:* You are also breaking the system configuration. We should change it to use the new parameter, but we should still accept the config values of the old parameter.
---
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: [FLINK-1679] rename degree of parallelism to p...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/488#issuecomment-83102692
With 236 files, that is hard to assess...
That is a point, in general, with humongous pull requests that touch 10 projects at the same time.
I would personally like another +1, from someone who verified that this is not API breaking. But again, PRs of this size are not easily reviewed...
---
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: [FLINK-1679] rename degree of parallelism to p...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/488#issuecomment-83100904
Are we sure that this is breaking no existing API. No method on DataSet or any descendant of it has been renamed? Also not on other user facing types?
---
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: [FLINK-1679] rename degree of parallelism to p...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/488#issuecomment-82251340
@uce @StephanEwen I agree with you two that it's better to keep the old API methods and mark them as deprecated while introducing the new ones. When the API has been stabilized and 1.0 comes up, we can remove the old 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.
---