Skip to content

Conversation

@mxm
Copy link
Contributor

@mxm mxm commented Mar 16, 2015

@uce
Copy link
Contributor

uce commented Mar 17, 2015

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.

@StephanEwen
Copy link
Contributor

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.

@mxm
Copy link
Contributor Author

mxm commented Mar 17, 2015

@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.

@mxm mxm force-pushed the parallelism branch 6 times, most recently from 9faf78d to cd347da Compare March 17, 2015 14:49
@mxm
Copy link
Contributor Author

mxm commented Mar 17, 2015

I've updated the pull request to include deprecation of the setters/getters and the config parameter.

@rmetzger
Copy link
Contributor

Thank you for the huge change! Our users will appreciate the more concise naming and improved documentation.

+1 to merge

@mxm
Copy link
Contributor Author

mxm commented Mar 18, 2015

Any objections against merging this?

@StephanEwen
Copy link
Contributor

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?

@StephanEwen
Copy link
Contributor

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...

mxm added a commit to mxm/flink that referenced this pull request Mar 18, 2015
mxm added a commit to mxm/flink that referenced this pull request Mar 18, 2015
@mxm
Copy link
Contributor Author

mxm commented Mar 19, 2015

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

@mxm mxm force-pushed the parallelism branch 5 times, most recently from ff29453 to 96900e3 Compare March 20, 2015 16:11
@mxm
Copy link
Contributor Author

mxm commented Mar 20, 2015

Any other opinions on the API breaking nature of these changes?

@StephanEwen
Copy link
Contributor

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.

@mxm mxm force-pushed the parallelism branch 2 times, most recently from 727537c to 1ba3a2d Compare March 20, 2015 16:26
@mxm
Copy link
Contributor Author

mxm commented Mar 20, 2015

@StephanEwen That's exactly what I did.

@mxm mxm force-pushed the parallelism branch 5 times, most recently from 54ef525 to 3c8de51 Compare March 21, 2015 11:12
mxm added 4 commits March 22, 2015 11:28
* 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
old config parameter can still be used

OLD
parallelization.degree.default

NEW
parallelism.default
@mxm mxm closed this Mar 23, 2015
@mxm mxm reopened this Mar 23, 2015
@mxm
Copy link
Contributor Author

mxm commented Mar 23, 2015

Merged in master with 126f9f7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants