You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by lamber-ken <gi...@git.apache.org> on 2018/07/03 18:43:20 UTC
[GitHub] flink pull request #6247: [FLINK-9730] [code refactor] fix access static via...
GitHub user lamber-ken opened a pull request:
https://github.com/apache/flink/pull/6247
[FLINK-9730] [code refactor] fix access static via class reference
## What is the purpose of the change
- fix access static via class reference
## Brief change log
- fix access static via class reference
## Verifying this change
- fix access static via class reference
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no)
- The serializers: (no)
- The runtime per-record code paths (performance sensitive): (no)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
- The S3 file system connector: (no)
## Documentation
- Does this pull request introduce a new feature? (no)
- If yes, how is the feature documented? (JavaDocs)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/lamber-ken/flink FLINK-9730
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/6247.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 #6247
----
commit d8278659cf6373aaa4d71320ad09d540955d396f
Author: lamber-ken <!...@...>
Date: 2018-07-03T18:36:43Z
[code refactore] access static via class reference
----
---
[GitHub] flink issue #6247: [FLINK-9730] [code refactor] fix access static via class ...
Posted by lamber-ken <gi...@git.apache.org>.
Github user lamber-ken commented on the issue:
https://github.com/apache/flink/pull/6247
@zentol, @tillrohrmann hi, cc. I fixed some cases access static via class reference
---
[GitHub] flink issue #6247: [FLINK-9730] [code refactor] fix access static via class ...
Posted by lamber-ken <gi...@git.apache.org>.
Github user lamber-ken commented on the issue:
https://github.com/apache/flink/pull/6247
@nekrassov, yan can open a separate JIRA. here's address [FLINK-JIRA](https://issues.apache.org/jira/projects/FLINK)
---
[GitHub] flink issue #6247: [FLINK-9730] [code refactor] fix access static via class ...
Posted by nekrassov <gi...@git.apache.org>.
Github user nekrassov commented on the issue:
https://github.com/apache/flink/pull/6247
there is a "Show outdated" button to see comments on the reverted commit.
I sent email to dev list, because I'm still not clear what was the original intent of the code, and how open() behaves.
---
[GitHub] flink pull request #6247: [FLINK-9730] [code refactor] fix access static via...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/6247
---
[GitHub] flink issue #6247: [FLINK-9730] [code refactor] fix access static via class ...
Posted by lamber-ken <gi...@git.apache.org>.
Github user lamber-ken commented on the issue:
https://github.com/apache/flink/pull/6247
@nekrassov, you are strict and right.
when I revert, the communication between us was removed by github.
---
[GitHub] flink pull request #6247: [FLINK-9730] [code refactor] fix access static via...
Posted by lamber-ken <gi...@git.apache.org>.
Github user lamber-ken commented on a diff in the pull request:
https://github.com/apache/flink/pull/6247#discussion_r200379627
--- Diff: flink-examples/flink-examples-streaming/src/main/java/org/apache/flink/streaming/examples/async/AsyncIOExample.java ---
@@ -179,7 +179,7 @@ public void close() throws Exception {
@Override
public void asyncInvoke(final Integer input, final ResultFuture<String> resultFuture) throws Exception {
- this.executorService.submit(new Runnable() {
+ executorService.submit(new Runnable() {
--- End diff --
no, the `executorService` is static
---
[GitHub] flink issue #6247: [FLINK-9730] [code refactor] fix access static via class ...
Posted by nekrassov <gi...@git.apache.org>.
Github user nekrassov commented on the issue:
https://github.com/apache/flink/pull/6247
lamber-ken, please let me know if you will open a separate JIRA and PR to make `random, executorService` non-static, remove `synchronized`, and remove `counter`. Or I can do that.
---
[GitHub] flink pull request #6247: [FLINK-9730] [code refactor] fix access static via...
Posted by nekrassov <gi...@git.apache.org>.
Github user nekrassov commented on a diff in the pull request:
https://github.com/apache/flink/pull/6247#discussion_r200413115
--- Diff: flink-examples/flink-examples-streaming/src/main/java/org/apache/flink/streaming/examples/async/AsyncIOExample.java ---
@@ -179,7 +179,7 @@ public void close() throws Exception {
@Override
public void asyncInvoke(final Integer input, final ResultFuture<String> resultFuture) throws Exception {
- this.executorService.submit(new Runnable() {
+ executorService.submit(new Runnable() {
--- End diff --
Sorry, my earlier comment about capital 'E' was wrong - I incorrectly assumed that submit() is a static method
---
[GitHub] flink pull request #6247: [FLINK-9730] [code refactor] fix access static via...
Posted by lamber-ken <gi...@git.apache.org>.
Github user lamber-ken commented on a diff in the pull request:
https://github.com/apache/flink/pull/6247#discussion_r200421156
--- Diff: flink-examples/flink-examples-streaming/src/main/java/org/apache/flink/streaming/examples/async/AsyncIOExample.java ---
@@ -179,7 +179,7 @@ public void close() throws Exception {
@Override
public void asyncInvoke(final Integer input, final ResultFuture<String> resultFuture) throws Exception {
- this.executorService.submit(new Runnable() {
+ executorService.submit(new Runnable() {
--- End diff --
don't feel sorry, you are welcome.
---
[GitHub] flink issue #6247: [FLINK-9730] [code refactor] fix access static via class ...
Posted by lamber-ken <gi...@git.apache.org>.
Github user lamber-ken commented on the issue:
https://github.com/apache/flink/pull/6247
@nekrassov, :+1: , you can fork `flink` project, then create a PR to fix.
---
[GitHub] flink pull request #6247: [FLINK-9730] [code refactor] fix access static via...
Posted by nekrassov <gi...@git.apache.org>.
Github user nekrassov commented on a diff in the pull request:
https://github.com/apache/flink/pull/6247#discussion_r200415625
--- Diff: flink-examples/flink-examples-streaming/src/main/java/org/apache/flink/streaming/examples/async/AsyncIOExample.java ---
@@ -116,8 +116,8 @@ public void cancel() {
private static class SampleAsyncFunction extends RichAsyncFunction<Integer, String> {
private static final long serialVersionUID = 2098635244857937717L;
- private static ExecutorService executorService;
- private static Random random;
--- End diff --
with this change, I think line 148 is no longer needed
---
[GitHub] flink pull request #6247: [FLINK-9730] [code refactor] fix access static via...
Posted by lamber-ken <gi...@git.apache.org>.
Github user lamber-ken commented on a diff in the pull request:
https://github.com/apache/flink/pull/6247#discussion_r200431068
--- Diff: flink-examples/flink-examples-streaming/src/main/java/org/apache/flink/streaming/examples/async/AsyncIOExample.java ---
@@ -116,8 +116,8 @@ public void cancel() {
private static class SampleAsyncFunction extends RichAsyncFunction<Integer, String> {
private static final long serialVersionUID = 2098635244857937717L;
- private static ExecutorService executorService;
- private static Random random;
--- End diff --
SampleAsyncFunction is used as singleton, so synchronize is needed
---
[GitHub] flink issue #6247: [FLINK-9730] [code refactor] fix access static via class ...
Posted by nekrassov <gi...@git.apache.org>.
Github user nekrassov commented on the issue:
https://github.com/apache/flink/pull/6247
https://issues.apache.org/jira/browse/FLINK-9784 created
---
[GitHub] flink issue #6247: [FLINK-9730] [code refactor] fix access static via class ...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/6247
for the AsyncIOExample, please revert the change to the `counter` field, and open a separate JIRA and PR. This issue is specifically about fixing static references.
---
[GitHub] flink pull request #6247: [FLINK-9730] [code refactor] fix access static via...
Posted by nekrassov <gi...@git.apache.org>.
Github user nekrassov commented on a diff in the pull request:
https://github.com/apache/flink/pull/6247#discussion_r200369787
--- Diff: flink-examples/flink-examples-streaming/src/main/java/org/apache/flink/streaming/examples/async/AsyncIOExample.java ---
@@ -179,7 +179,7 @@ public void close() throws Exception {
@Override
public void asyncInvoke(final Integer input, final ResultFuture<String> resultFuture) throws Exception {
- this.executorService.submit(new Runnable() {
+ executorService.submit(new Runnable() {
--- End diff --
Did you mean to write "ExecutorService" - with the capital 'E'?
---
[GitHub] flink issue #6247: [FLINK-9730] [code refactor] fix access static via class ...
Posted by lamber-ken <gi...@git.apache.org>.
Github user lamber-ken commented on the issue:
https://github.com/apache/flink/pull/6247
@nekrassov, @zentol, I'll revert it, just fix static references.
I didn't pay attention to the problem of concurrency at first.
---
[GitHub] flink pull request #6247: [FLINK-9730] [code refactor] fix access static via...
Posted by lamber-ken <gi...@git.apache.org>.
Github user lamber-ken commented on a diff in the pull request:
https://github.com/apache/flink/pull/6247#discussion_r200398562
--- Diff: flink-examples/flink-examples-streaming/src/main/java/org/apache/flink/streaming/examples/async/AsyncIOExample.java ---
@@ -179,7 +179,7 @@ public void close() throws Exception {
@Override
public void asyncInvoke(final Integer input, final ResultFuture<String> resultFuture) throws Exception {
- this.executorService.submit(new Runnable() {
+ executorService.submit(new Runnable() {
--- End diff --
@nekrassov, right.
These two variables are private(`random`,`executorService`), so can remove static define. do you think so?
---
[GitHub] flink pull request #6247: [FLINK-9730] [code refactor] fix access static via...
Posted by lamber-ken <gi...@git.apache.org>.
Github user lamber-ken commented on a diff in the pull request:
https://github.com/apache/flink/pull/6247#discussion_r200421209
--- Diff: flink-examples/flink-examples-streaming/src/main/java/org/apache/flink/streaming/examples/async/AsyncIOExample.java ---
@@ -116,8 +116,8 @@ public void cancel() {
private static class SampleAsyncFunction extends RichAsyncFunction<Integer, String> {
private static final long serialVersionUID = 2098635244857937717L;
- private static ExecutorService executorService;
- private static Random random;
--- End diff --
right, line 118, not line 148
---
[GitHub] flink pull request #6247: [FLINK-9730] [code refactor] fix access static via...
Posted by nekrassov <gi...@git.apache.org>.
Github user nekrassov commented on a diff in the pull request:
https://github.com/apache/flink/pull/6247#discussion_r200423552
--- Diff: flink-examples/flink-examples-streaming/src/main/java/org/apache/flink/streaming/examples/async/AsyncIOExample.java ---
@@ -116,8 +116,8 @@ public void cancel() {
private static class SampleAsyncFunction extends RichAsyncFunction<Integer, String> {
private static final long serialVersionUID = 2098635244857937717L;
- private static ExecutorService executorService;
- private static Random random;
--- End diff --
No, my point is: if we are making executorService and random instance-specific - then we don't need to synchronize on class (lines 148 and 163).
If we want to keep executorService and random as static, then we need to make counter static too (line 122).
I think having all three (executorService, random, counter) as static is best.
Having executorService and random as static, but counter as instance-specific will not work when someone creates a second instance of SampleAsyncFunction. In second SampleAsyncFunction the counter will be 0 and we will re-intialize static executorService and random, thus interfering with the first SampleAsyncFunction object.
---
[GitHub] flink pull request #6247: [FLINK-9730] [code refactor] fix access static via...
Posted by nekrassov <gi...@git.apache.org>.
Github user nekrassov commented on a diff in the pull request:
https://github.com/apache/flink/pull/6247#discussion_r200393008
--- Diff: flink-examples/flink-examples-streaming/src/main/java/org/apache/flink/streaming/examples/async/AsyncIOExample.java ---
@@ -179,7 +179,7 @@ public void close() throws Exception {
@Override
public void asyncInvoke(final Integer input, final ResultFuture<String> resultFuture) throws Exception {
- this.executorService.submit(new Runnable() {
+ executorService.submit(new Runnable() {
--- End diff --
see 10.2 in "Java Code Conventions" (http://www.oracle.com/technetwork/java/codeconventions-150003.pdf)
---
[GitHub] flink pull request #6247: [FLINK-9730] [code refactor] fix access static via...
Posted by nekrassov <gi...@git.apache.org>.
Github user nekrassov commented on a diff in the pull request:
https://github.com/apache/flink/pull/6247#discussion_r200434401
--- Diff: flink-examples/flink-examples-streaming/src/main/java/org/apache/flink/streaming/examples/async/AsyncIOExample.java ---
@@ -116,8 +116,8 @@ public void cancel() {
private static class SampleAsyncFunction extends RichAsyncFunction<Integer, String> {
private static final long serialVersionUID = 2098635244857937717L;
- private static ExecutorService executorService;
- private static Random random;
--- End diff --
What makes it a singleton?..
Nothing stops me from adding a call in main():
`AsyncFunction<Integer, String> function2 = new SampleAsyncFunction(sleepFactor2, failRatio2, shutdownWaitTS2);`
IMHO, we need to put back "static" on executorService and random. And add "static" to counter.
---