Skip to content

Conversation

@smarthi
Copy link
Member

@smarthi smarthi commented Mar 24, 2015

Fixes Flink-1780

@fhueske
Copy link
Contributor

fhueske commented Mar 24, 2015

@smarthi Thanks for the PR!
LGTM, except for a few lines where I spotted space indentions that do not comply with our code style.

@smarthi
Copy link
Member Author

smarthi commented Mar 24, 2015

@fhueske yeah noticed that too, will update the PR

@smarthi
Copy link
Member Author

smarthi commented Mar 24, 2015

@fhueske i think we shuld be LGTM now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this change improves the readability of the code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. please feel free to revert this change.

@mxm
Copy link
Contributor

mxm commented Mar 25, 2015

+1 This rename makes a lot of sense. Looks good to me apart from the minor comments.

@mxm
Copy link
Contributor

mxm commented Mar 25, 2015

The documentation also needs to be adapted to use the new interface name.

@smarthi If you don't mind I will merge your commit with my remarks addressed.

@smarthi
Copy link
Member Author

smarthi commented Mar 25, 2015

@mxm please go ahead

@asfgit asfgit closed this in 033c69f Mar 25, 2015
@smarthi smarthi deleted the Flink-1780 branch March 25, 2015 16:21
@mxm
Copy link
Contributor

mxm commented Mar 25, 2015

Thank you for the pull request!

bhatsachin pushed a commit to bhatsachin/flink that referenced this pull request May 5, 2015
marthavk pushed a commit to marthavk/flink that referenced this pull request Jun 9, 2015
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