You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by yanghua <gi...@git.apache.org> on 2018/04/28 02:47:50 UTC
[GitHub] flink pull request #5933: [FLINK-9125] MiniClusterResource should set CoreOp...
GitHub user yanghua opened a pull request:
https://github.com/apache/flink/pull/5933
[FLINK-9125] MiniClusterResource should set CoreOptions.TMP_DIRS
## What is the purpose of the change
*This pull request sets the `CoreOptions.TMP_DIRS` for MiniClusterResource*
## Brief change log
- *Initialized a `TemporaryFolder` instance and set `CoreOptions.TMP_DIRS` config*
## Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (yes / **no**)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes / **no**)
- The serializers: (yes / **no** / don't know)
- The runtime per-record code paths (performance sensitive): (yes / **no** / don't know)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / **no** / don't know)
- The S3 file system connector: (yes / **no** / don't know)
## Documentation
- Does this pull request introduce a new feature? (yes / **no**)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / **not documented**)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/yanghua/flink FLINK-9125
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/5933.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 #5933
----
commit 1f8b04fe4a80f88bfccc5246bf57ce2da84a5a70
Author: yanghua <ya...@...>
Date: 2018-04-28T02:43:27Z
[FLINK-9125] MiniClusterResource should set CoreOptions.TMP_DIRS
----
---
[GitHub] flink issue #5933: [FLINK-9125] MiniClusterResource should set CoreOptions.T...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/5933
@zentol @GJL removed unnecessary annotation, please review it again thanks.
---
[GitHub] flink pull request #5933: [FLINK-9125] MiniClusterResource should set CoreOp...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on a diff in the pull request:
https://github.com/apache/flink/pull/5933#discussion_r184892264
--- Diff: flink-test-utils-parent/flink-test-utils/src/main/java/org/apache/flink/test/util/MiniClusterResource.java ---
@@ -149,6 +155,7 @@ public void before() throws Exception {
@Override
public void after() {
+ temporaryFolder.delete();
--- End diff --
No need to call this explicitly:
```
/**
* Delete all files and folders under the temporary folder. Usually not
* called directly, since it is automatically applied by the {@link Rule}
*/
public void delete() {
if (folder != null) {
recursiveDelete(folder);
}
}
```
---
[GitHub] flink pull request #5933: [FLINK-9125] MiniClusterResource should set CoreOp...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5933#discussion_r184928655
--- Diff: flink-test-utils-parent/flink-test-utils/src/main/java/org/apache/flink/test/util/MiniClusterResource.java ---
@@ -59,6 +61,9 @@
public static final String NEW_CODEBASE = "new";
+ @Rule
--- End diff --
does this annotation actually work in arbitrary classes?
---
[GitHub] flink pull request #5933: [FLINK-9125] MiniClusterResource should set CoreOp...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5933#discussion_r186345933
--- Diff: flink-test-utils-parent/flink-test-utils/src/main/java/org/apache/flink/test/util/MiniClusterResource.java ---
@@ -59,6 +60,8 @@
public static final String NEW_CODEBASE = "new";
+ public final TemporaryFolder temporaryFolder = new TemporaryFolder();
--- End diff --
can be private
---
[GitHub] flink pull request #5933: [FLINK-9125] MiniClusterResource should set CoreOp...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on a diff in the pull request:
https://github.com/apache/flink/pull/5933#discussion_r186347318
--- Diff: flink-test-utils-parent/flink-test-utils/src/main/java/org/apache/flink/test/util/MiniClusterResource.java ---
@@ -59,6 +60,8 @@
public static final String NEW_CODEBASE = "new";
+ public final TemporaryFolder temporaryFolder = new TemporaryFolder();
--- End diff --
changed
---
[GitHub] flink pull request #5933: [FLINK-9125] MiniClusterResource should set CoreOp...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on a diff in the pull request:
https://github.com/apache/flink/pull/5933#discussion_r184892245
--- Diff: flink-test-utils-parent/flink-test-utils/src/main/java/org/apache/flink/test/util/MiniClusterResource.java ---
@@ -137,6 +142,7 @@ public int getWebUIPort() {
@Override
public void before() throws Exception {
+ temporaryFolder.create();
--- End diff --
It's wrong to call `create`:
```
/**
* for testing purposes only. Do not use.
*/
public void create() throws IOException {
folder = createTemporaryFolderIn(parentFolder);
}
```
---
[GitHub] flink pull request #5933: [FLINK-9125] MiniClusterResource should set CoreOp...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on a diff in the pull request:
https://github.com/apache/flink/pull/5933#discussion_r186346559
--- Diff: flink-test-utils-parent/flink-test-utils/src/main/java/org/apache/flink/test/util/MiniClusterResource.java ---
@@ -59,6 +60,8 @@
public static final String NEW_CODEBASE = "new";
+ public final TemporaryFolder temporaryFolder = new TemporaryFolder();
--- End diff --
changing...
---
[GitHub] flink pull request #5933: [FLINK-9125] MiniClusterResource should set CoreOp...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on a diff in the pull request:
https://github.com/apache/flink/pull/5933#discussion_r185596918
--- Diff: flink-test-utils-parent/flink-test-utils/src/main/java/org/apache/flink/test/util/MiniClusterResource.java ---
@@ -137,6 +142,7 @@ public int getWebUIPort() {
@Override
public void before() throws Exception {
+ temporaryFolder.create();
--- End diff --
good point
---
[GitHub] flink issue #5933: [FLINK-9125] MiniClusterResource should set CoreOptions.T...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/5933
merging.
---
[GitHub] flink pull request #5933: [FLINK-9125] MiniClusterResource should set CoreOp...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/5933
---
[GitHub] flink issue #5933: [FLINK-9125] MiniClusterResource should set CoreOptions.T...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/5933
cc @zentol
---
[GitHub] flink pull request #5933: [FLINK-9125] MiniClusterResource should set CoreOp...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5933#discussion_r184928993
--- Diff: flink-test-utils-parent/flink-test-utils/src/main/java/org/apache/flink/test/util/MiniClusterResource.java ---
@@ -137,6 +142,7 @@ public int getWebUIPort() {
@Override
public void before() throws Exception {
+ temporaryFolder.create();
--- End diff --
could be necessary if the `@Rule` annotation doesn't work in arbitrary (i..e. non-test) classes. Then we have to manage it manually, with `create()` and `before()`.
---
[GitHub] flink issue #5933: [FLINK-9125] MiniClusterResource should set CoreOptions.T...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/5933
@zentol please review again, if you have time.
---