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-6177] Add support for "Distributed Cache" in streaming applica… #3741

Conversation

zohar-mizrahi
Copy link

@zohar-mizrahi zohar-mizrahi commented Apr 19, 2017

…tions

Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the How To Contribute guide.
In addition to going through the list, please provide a meaningful description of your changes.

  • General

    • The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
    • 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

@StephanEwen
Copy link
Contributor

Thanks for contributing this, the added functionality looks good.

I would prefer to add this change without changing the dependencies and test base classes. You could for example change the test to throw an exception in the "validator function" if the word is not in the cache file. That way you do not need to "collect back" the data.

Minor comment: Generating the input from a collection rather than a file makes the tests usually a bit more lightweight. In all newer tests, we try to do that.

@zohar-mizrahi
Copy link
Author

No problem. Will follow it and will create a new pull request.

@zentol
Copy link
Contributor

zentol commented Apr 19, 2017

@zohar-pm You don't have to open a new one, feel free to update the branch in this one.

@StephanEwen
Copy link
Contributor

Looks quite good now.

If I can ask you for one more followup: To have faster tests, it would be good to add the streaming distributed cache test and the batch distributed cache test to the same file.

Can you change the DistributedCacheTest to extend StreamingMultipleProgramsTestBase and put your test in there as well? That will cause only one distributed mini cluster to be spawned, which typically saves 1-2 secs in tests. May not seem much, but it adds up over the 1000s of tests Flink has by now...

Copy link
Contributor

@zentol zentol left a comment

Choose a reason for hiding this comment

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

I think this is good to merge now, bar 2 minor code style issues.

As a general note, it's recommended to add a comment when updating the PR. We don't get any notifications for pushed commits.

* @param filePath The path of the file, as a URI (e.g. "file:///some/path" or "hdfs://host:port/and/path")
* @param name The name under which the file is registered.
*/
public void registerCachedFile(String filePath, String name){
Copy link
Contributor

Choose a reason for hiding this comment

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

missing space before {.

* @param name The name under which the file is registered.
* @param executable flag indicating whether the file should be executable
*/
public void registerCachedFile(String filePath, String name, boolean executable){
Copy link
Contributor

Choose a reason for hiding this comment

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

missing space before }.

@zentol
Copy link
Contributor

zentol commented May 3, 2017

merging. will add the missing space while I'm doing it.

zentol pushed a commit to zentol/flink that referenced this pull request May 3, 2017
zentol pushed a commit to zentol/flink that referenced this pull request May 3, 2017
zentol pushed a commit to zentol/flink that referenced this pull request May 4, 2017
@asfgit asfgit closed this in 3655dee May 4, 2017
fanyon pushed a commit to fanyon/flink that referenced this pull request May 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants