[GitHub] flink pull request #6247: [FLINK-9730] [code refactor] fix access static via...
Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/6247 ---
[GitHub] flink pull request #6247: [FLINK-9730] [code refactor] fix access static via...
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 {
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 function2 = new
SampleAsyncFunction(sleepFactor2, failRatio2, shutdownWaitTS2);`
IMHO, we need to put back "static" on executorService and random. And add
"static" to counter.
---
[GitHub] flink pull request #6247: [FLINK-9730] [code refactor] fix access static via...
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 {
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 pull request #6247: [FLINK-9730] [code refactor] fix access static via...
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 {
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...
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 {
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...
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 resultFuture) throws Exception {
- this.executorService.submit(new Runnable() {
+ executorService.submit(new Runnable() {
--- End diff --
don't feel sorry, you are welcome.
---
[GitHub] flink pull request #6247: [FLINK-9730] [code refactor] fix access static via...
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 {
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...
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 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...
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 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...
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 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...
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 resultFuture) throws Exception {
- this.executorService.submit(new Runnable() {
+ executorService.submit(new Runnable() {
--- End diff --
no, the `executorService` is static
---
[GitHub] flink pull request #6247: [FLINK-9730] [code refactor] fix access static via...
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 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 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 ---
