Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FLINK-5175] StreamExecutionEnvironment's set function return this instead of void #2874

Closed
wants to merge 1 commit into from

Conversation

shijinkui
Copy link
Contributor

@shijinkui shijinkui commented Nov 26, 2016

StreamExecutionEnvironment's set function return this instead of void

for example :

public void setNumberOfExecutionRetries(int numberOfExecutionRetries) {
config.setNumberOfExecutionRetries(numberOfExecutionRetries);
}

change to:

public StreamExecutionEnvironment setNumberOfExecutionRetries(int numberOfExecutionRetries) {
config.setNumberOfExecutionRetries(numberOfExecutionRetries);
return this;
}

  • General

    • The pull request references the related JIRA issue ("[FLINK-5175] StreamExecutionEnvironment's set function return this instead of void")
    • 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

@fhueske
Copy link
Contributor

fhueske commented Nov 26, 2016

The PR changes several @Public and @PublicEvolving methods.

We do not touch @public interfaces for 1.x releases and also try to avoid to modify @PublicEvolving interface except for very good reasons. We have a Maven plugin that prevents changes that break the binary compatibility of methods and classes annotated with @Public.
By building the code before opening a pull request, you can early detect such issues.

I don't think that we can and should merge this PR.

@shijinkui
Copy link
Contributor Author

shijinkui commented Nov 26, 2016

The PR changes several @public and @PublicEvolving methods.

hi, @fhueske
Today when set some config, find some function cant't chained one by one. So want to complete all the function needed return this conveniently. If chained setting config, the setting will be together, avoid setting config everywhere.

It's only better used, not important. You can close this.

@fhueske
Copy link
Contributor

fhueske commented Nov 28, 2016

Thanks for the proposal @shijinkui. I agree that the changes might improve the usability a bit for some users. However, the improvements are not significant enough to break the API, IMO.

I'll close this PR and the issue.
Thanks, Fabian

@StephanEwen
Copy link
Contributor

@shijinkui I think these changes would be great for Flink 2.0

Can you add them to this list here? That is where we collect all API-.breaking changes that we want to add.
https://issues.apache.org/jira/browse/FLINK-3957

Thanks!

@shijinkui shijinkui changed the title [FLINK-5167] StreamExecutionEnvironment set function return this in… [FLINK-5175] StreamExecutionEnvironment's set function return this instead of void Nov 28, 2016
@shijinkui
Copy link
Contributor Author

Can you add them to this list here? That is where we collect all API-.breaking changes that we want to add.

Done.

@asfgit asfgit closed this in db441de Nov 29, 2016
liuyuzhong pushed a commit to liuyuzhong/flink that referenced this pull request Dec 2, 2016
…eneration.

This closes apache#2884
This closes apache#2874 (closing PR with Public API breaking changes)
liuyuzhong pushed a commit to liuyuzhong/flink that referenced this pull request Dec 5, 2016
…eneration.

This closes apache#2884
This closes apache#2874 (closing PR with Public API breaking changes)
static-max pushed a commit to static-max/flink that referenced this pull request Dec 13, 2016
…eneration.

This closes apache#2884
This closes apache#2874 (closing PR with Public API breaking changes)
joseprupi pushed a commit to joseprupi/flink that referenced this pull request Feb 12, 2017
…eneration.

This closes apache#2884
This closes apache#2874 (closing PR with Public API breaking changes)
hequn8128 pushed a commit to hequn8128/flink that referenced this pull request Jun 22, 2017
…eneration.

This closes apache#2884
This closes apache#2874 (closing PR with Public API breaking changes)
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