[jira] [Commented] (FLINK-16503) Don't assign ScheduledExecutorService into variables of ExecutorService or Executor types
[
https://issues.apache.org/jira/browse/FLINK-16503?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17054816#comment-17054816
]
Roman Leventov commented on FLINK-16503:
No
> Don't assign ScheduledExecutorService into variables of ExecutorService or
> Executor types
> -
>
> Key: FLINK-16503
> URL: https://issues.apache.org/jira/browse/FLINK-16503
> Project: Flink
> Issue Type: Improvement
>Reporter: Roman Leventov
>Priority: Minor
>
> There is an unnecessary efficiency cost to assigning ScheduledExecutorService
> (typically, ScheduledThreadPoolExecutor) into variables of ExecutorService or
> Executor types.
> Currently, there is one such occurrence in production code, in
> TaskManagerRunner, and three more in tests.
> They could be found using IntelliJ's Structural search pattern (and
> Structural Search inspection):
> {{$x$ = $y$;}}
> Where the Type of {{$x$}} is set to {{ExecutorService|Executor}} ("within
> type hierarchy" checkbox is off), and the Type of {{$y$}} is set to
> {{ScheduledThreadPoolExecutor }}("within type hierarchy" checkbox is on).
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (FLINK-16503) Don't assign ScheduledExecutorService into variables of ExecutorService or Executor types
[
https://issues.apache.org/jira/browse/FLINK-16503?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17054814#comment-17054814
]
Roman Leventov commented on FLINK-16503:
FYI: I included ForkJoinPool earlier as an unnecessary rhs expression, but it
is actually not seriously warranted, unlike ScheduledThreadPoolExecutor. JDK
maintainers even consider to make Executors.newFixedTheadPool() to return
ForkJoinPool in the future:
[http://cs.oswego.edu/pipermail/concurrency-interest/2020-February/017061.html]
> Don't assign ScheduledExecutorService into variables of ExecutorService or
> Executor types
> -
>
> Key: FLINK-16503
> URL: https://issues.apache.org/jira/browse/FLINK-16503
> Project: Flink
> Issue Type: Improvement
>Reporter: Roman Leventov
>Priority: Minor
>
> There is an unnecessary efficiency cost to assigning ScheduledExecutorService
> (typically, ScheduledThreadPoolExecutor) into variables of ExecutorService or
> Executor types.
> Currently, there is one such occurrence in production code, in
> TaskManagerRunner, and three more in tests.
> They could be found using IntelliJ's Structural search pattern (and
> Structural Search inspection):
> {{$x$ = $y$;}}
> Where the Type of {{$x$}} is set to {{ExecutorService|Executor}} ("within
> type hierarchy" checkbox is off), and the Type of {{$y$}} is set to
> {{ScheduledThreadPoolExecutor }}("within type hierarchy" checkbox is on).
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (FLINK-16503) Don't assign ScheduledExecutorService into variables of ExecutorService or Executor types
[
https://issues.apache.org/jira/browse/FLINK-16503?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17054811#comment-17054811
]
Roman Leventov commented on FLINK-16503:
[~tison] yes, this is the sole change in Java code that should be done, in
TaskManagerRunner constructor. Apart from some CI/static analysis config that
should automatically prevent such regressions in the future.
> Don't assign ScheduledExecutorService into variables of ExecutorService or
> Executor types
> -
>
> Key: FLINK-16503
> URL: https://issues.apache.org/jira/browse/FLINK-16503
> Project: Flink
> Issue Type: Improvement
>Reporter: Roman Leventov
>Priority: Minor
>
> There is an unnecessary efficiency cost to assigning ScheduledExecutorService
> (typically, ScheduledThreadPoolExecutor) into variables of ExecutorService or
> Executor types.
> Currently, there is one such occurrence in production code, in
> TaskManagerRunner, and three more in tests.
> They could be found using IntelliJ's Structural search pattern (and
> Structural Search inspection):
> {{$x$ = $y$;}}
> Where the Type of {{$x$}} is set to {{ExecutorService|Executor}} ("within
> type hierarchy" checkbox is off), and the Type of {{$y$}} is set to
> {{ScheduledThreadPoolExecutor }}("within type hierarchy" checkbox is on).
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Updated] (FLINK-16503) Don't assign ScheduledExecutorService into variables of ExecutorService or Executor types
[
https://issues.apache.org/jira/browse/FLINK-16503?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Roman Leventov updated FLINK-16503:
---
Description:
There is an unnecessary efficiency cost to assigning ScheduledExecutorService
(typically, ScheduledThreadPoolExecutor) into variables of ExecutorService or
Executor types.
Currently, there is one such occurrence in production code, in
TaskManagerRunner, and three more in tests.
They could be found using IntelliJ's Structural search pattern (and Structural
Search inspection):
{{$x$ = $y$;}}
Where the Type of {{$x$}} is set to {{ExecutorService|Executor}} ("within type
hierarchy" checkbox is off), and the Type of {{$y$}} is set to
{{ScheduledThreadPoolExecutor }}("within type hierarchy" checkbox is on).
was:
There is an unnecessary efficiency cost to assigning ScheduledExecutorService
(typically, ScheduledThreadPoolExecutor) into variables of ExecutorService or
Executor types.
Currently, there is one such occurrence in production code, in
TaskManagerRunner, and three more in tests.
They could be found using IntelliJ's Structural search pattern (and Structural
Search inspection):
{{$x$ = $y$;}}
Where the Type of {{$x$}} is set to {{ExecutorService|Executor}} ("within type
hierarchy" checkbox is off), and the Type of {{$y$}} is set to
{{ScheduledThreadPoolExecutor|ForkJoinPool}} ("within type hierarchy" checkbox
is on).
> Don't assign ScheduledExecutorService into variables of ExecutorService or
> Executor types
> -
>
> Key: FLINK-16503
> URL: https://issues.apache.org/jira/browse/FLINK-16503
> Project: Flink
> Issue Type: Improvement
>Reporter: Roman Leventov
>Priority: Minor
>
> There is an unnecessary efficiency cost to assigning ScheduledExecutorService
> (typically, ScheduledThreadPoolExecutor) into variables of ExecutorService or
> Executor types.
> Currently, there is one such occurrence in production code, in
> TaskManagerRunner, and three more in tests.
> They could be found using IntelliJ's Structural search pattern (and
> Structural Search inspection):
> {{$x$ = $y$;}}
> Where the Type of {{$x$}} is set to {{ExecutorService|Executor}} ("within
> type hierarchy" checkbox is off), and the Type of {{$y$}} is set to
> {{ScheduledThreadPoolExecutor }}("within type hierarchy" checkbox is on).
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Comment Edited] (FLINK-16503) Don't assign ScheduledExecutorService into variables of ExecutorService or Executor types
[
https://issues.apache.org/jira/browse/FLINK-16503?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17054800#comment-17054800
]
Roman Leventov edited comment on FLINK-16503 at 3/9/20, 9:40 AM:
-
Another pattern that should be checked in CI static analysis is a "Java - Class
Member" pattern:
{{$Type$ $x$ = $y$;}}
Where the "Text" of {{$Type$}} is {{ExecutorService|Executor}} and the "Type"
of {{$y$}} is {{ScheduledThreadPoolExecutor}}, within type hierarchy.
Currently, this pattern doesn't result in violation findings in production code.
was (Author: leventov):
Another pattern that should be checked in CI static analysis is a "Java - Class
Member" pattern:
{{$Type$ $x$ = $y$;}}
Where the type of {{$Type$}} is {{ExecutorService|Executor}} and the type of
{{$y$}} is {{ScheduledThreadPoolExecutor|ForkJoinPool}}, within type hierarchy.
Currently, this pattern doesn't result in violation findings in production code.
> Don't assign ScheduledExecutorService into variables of ExecutorService or
> Executor types
> -
>
> Key: FLINK-16503
> URL: https://issues.apache.org/jira/browse/FLINK-16503
> Project: Flink
> Issue Type: Improvement
>Reporter: Roman Leventov
>Priority: Minor
>
> There is an unnecessary efficiency cost to assigning ScheduledExecutorService
> (typically, ScheduledThreadPoolExecutor) into variables of ExecutorService or
> Executor types.
> Currently, there is one such occurrence in production code, in
> TaskManagerRunner, and three more in tests.
> They could be found using IntelliJ's Structural search pattern (and
> Structural Search inspection):
> {{$x$ = $y$;}}
> Where the Type of {{$x$}} is set to {{ExecutorService|Executor}} ("within
> type hierarchy" checkbox is off), and the Type of {{$y$}} is set to
> {{ScheduledThreadPoolExecutor }}("within type hierarchy" checkbox is on).
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (FLINK-16503) Don't assign ScheduledExecutorService into variables of ExecutorService or Executor types
[
https://issues.apache.org/jira/browse/FLINK-16503?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17054804#comment-17054804
]
Roman Leventov commented on FLINK-16503:
[~tison] ScheduledThreadPoolExecutor uses a priority queue instead of a simple
queue like ThreadPoolExecutor, which results in higher memory usage for stored
jobs in the queue and longer submit() and dequeue.
> Don't assign ScheduledExecutorService into variables of ExecutorService or
> Executor types
> -
>
> Key: FLINK-16503
> URL: https://issues.apache.org/jira/browse/FLINK-16503
> Project: Flink
> Issue Type: Improvement
>Reporter: Roman Leventov
>Priority: Minor
>
> There is an unnecessary efficiency cost to assigning ScheduledExecutorService
> (typically, ScheduledThreadPoolExecutor) into variables of ExecutorService or
> Executor types.
> Currently, there is one such occurrence in production code, in
> TaskManagerRunner, and three more in tests.
> They could be found using IntelliJ's Structural search pattern (and
> Structural Search inspection):
> {{$x$ = $y$;}}
> Where the Type of {{$x$}} is set to {{ExecutorService|Executor}} ("within
> type hierarchy" checkbox is off), and the Type of {{$y$}} is set to
> {{ScheduledThreadPoolExecutor|ForkJoinPool}} ("within type hierarchy"
> checkbox is on).
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (FLINK-16503) Don't assign ScheduledExecutorService into variables of ExecutorService or Executor types
[
https://issues.apache.org/jira/browse/FLINK-16503?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17054800#comment-17054800
]
Roman Leventov commented on FLINK-16503:
Another pattern that should be checked in CI static analysis is a "Java - Class
Member" pattern:
{{$Type$ $x$ = $y$;}}
Where the type of {{$Type$}} is {{ExecutorService|Executor}} and the type of
{{$y$}} is {{ScheduledThreadPoolExecutor|ForkJoinPool}}, within type hierarchy.
Currently, this pattern doesn't result in violation findings in production code.
> Don't assign ScheduledExecutorService into variables of ExecutorService or
> Executor types
> -
>
> Key: FLINK-16503
> URL: https://issues.apache.org/jira/browse/FLINK-16503
> Project: Flink
> Issue Type: Improvement
>Reporter: Roman Leventov
>Priority: Minor
>
> There is an unnecessary efficiency cost to assigning ScheduledExecutorService
> (typically, ScheduledThreadPoolExecutor) into variables of ExecutorService or
> Executor types.
> Currently, there is one such occurrence in production code, in
> TaskManagerRunner, and three more in tests.
> They could be found using IntelliJ's Structural search pattern (and
> Structural Search inspection):
> {{$x$ = $y$;}}
> Where the Type of {{$x$}} is set to {{ExecutorService|Executor}} ("within
> type hierarchy" checkbox is off), and the Type of {{$y$}} is set to
> {{ScheduledThreadPoolExecutor|ForkJoinPool}} ("within type hierarchy"
> checkbox is on).
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Updated] (FLINK-16503) Don't assign ScheduledExecutorService into variables of ExecutorService or Executor types
[
https://issues.apache.org/jira/browse/FLINK-16503?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Roman Leventov updated FLINK-16503:
---
Description:
There is an unnecessary efficiency cost to assigning ScheduledExecutorService
(typically, ScheduledThreadPoolExecutor) into variables of ExecutorService or
Executor types.
Currently, there is one such occurrence in production code, in
TaskManagerRunner, and three more in tests.
They could be found using IntelliJ's Structural search pattern (and Structural
Search inspection):
{{$x$ = $y$;}}
Where the Type of {{$x$}} is set to {{ExecutorService|Executor}} ("within type
hierarchy" checkbox is off), and the Type of {{$y$}} is set to
{{ScheduledThreadPoolExecutor|ForkJoinPool}} ("within type hierarchy" checkbox
is on).
was:
There is an unnecessary efficiency cost to assigning ScheduledExecutorService
(typically, ScheduledThreadPoolExecutor) into variables of ExecutorService or
Executor types.
Currently, there is one such occurrence in production code, in
TaskManagerRunner, and three more in tests.
They could be found using IntelliJ's Structural search pattern (and Structural
Search inspection):
{{$x$ = $y$;}}
Where the Type of {{$x$}} is set to {{ExecutorService|Executor}} ("within type
hierarchy" checkbox is off), and the Type of {{$y$}} is set to
{{ScheduledThreadPoolExecutor}} ("within type hierarchy" checkbox is on).
> Don't assign ScheduledExecutorService into variables of ExecutorService or
> Executor types
> -
>
> Key: FLINK-16503
> URL: https://issues.apache.org/jira/browse/FLINK-16503
> Project: Flink
> Issue Type: Improvement
>Reporter: Roman Leventov
>Priority: Minor
>
> There is an unnecessary efficiency cost to assigning ScheduledExecutorService
> (typically, ScheduledThreadPoolExecutor) into variables of ExecutorService or
> Executor types.
> Currently, there is one such occurrence in production code, in
> TaskManagerRunner, and three more in tests.
> They could be found using IntelliJ's Structural search pattern (and
> Structural Search inspection):
> {{$x$ = $y$;}}
> Where the Type of {{$x$}} is set to {{ExecutorService|Executor}} ("within
> type hierarchy" checkbox is off), and the Type of {{$y$}} is set to
> {{ScheduledThreadPoolExecutor|ForkJoinPool}} ("within type hierarchy"
> checkbox is on).
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Created] (FLINK-16503) Don't assign ScheduledExecutorService into variables of ExecutorService or Executor types
Roman Leventov created FLINK-16503:
--
Summary: Don't assign ScheduledExecutorService into variables of
ExecutorService or Executor types
Key: FLINK-16503
URL: https://issues.apache.org/jira/browse/FLINK-16503
Project: Flink
Issue Type: Improvement
Reporter: Roman Leventov
There is an unnecessary efficiency cost to assigning ScheduledExecutorService
(typically, ScheduledThreadPoolExecutor) into variables of ExecutorService or
Executor types.
Currently, there is one such occurrence in production code, in
TaskManagerRunner, and three more in tests.
They could be found using IntelliJ's Structural search pattern (and Structural
Search inspection):
{{$x$ = $y$;}}
Where the Type of {{$x$}} is set to {{ExecutorService|Executor}} ("within type
hierarchy" checkbox is off), and the Type of {{$y$}} is set to
{{ScheduledThreadPoolExecutor}} ("within type hierarchy" checkbox is on).
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (FLINK-16365) awaitTermination() result is not checked
[ https://issues.apache.org/jira/browse/FLINK-16365?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17048848#comment-17048848 ] Roman Leventov commented on FLINK-16365: To find all these places, one can use `$x$.awaitTermination($y$, $z$);` structural search pattern in IntelliJ IDEA. > awaitTermination() result is not checked > > > Key: FLINK-16365 > URL: https://issues.apache.org/jira/browse/FLINK-16365 > Project: Flink > Issue Type: Improvement >Reporter: Roman Leventov >Priority: Minor > > There are three places in production code where awaitTermination() result is > not checked: BlockingGrpcPubSubSubscriber (io.grpc.ManagedChannel), > PubSubSink (ManagedChannel), and FileCache (ExecutorService). > Calling awaitTermination() without checking the result seems to make little > sense to me. > If it's genuinely important to await termination, e. g. for concurrency > reasons, or because we are awaiting heavy resource release and if the > resource is not released we have a resource leak, then it seems reasonable to > at least check the result of awaitTermination() and log a warning if the > result is negative, allowing to debug potential problem in the future. > Otherwise, if we don't really care about awaiting termination, then maybe > it's better to not call awaitTermination() at all. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Created] (FLINK-16365) awaitTermination() result is not checked
Roman Leventov created FLINK-16365: -- Summary: awaitTermination() result is not checked Key: FLINK-16365 URL: https://issues.apache.org/jira/browse/FLINK-16365 Project: Flink Issue Type: Improvement Reporter: Roman Leventov There are three places in production code where awaitTermination() result is not checked: BlockingGrpcPubSubSubscriber (io.grpc.ManagedChannel), PubSubSink (ManagedChannel), and FileCache (ExecutorService). Calling awaitTermination() without checking the result seems to make little sense to me. If it's genuinely important to await termination, e. g. for concurrency reasons, or because we are awaiting heavy resource release and if the resource is not released we have a resource leak, then it seems reasonable to at least check the result of awaitTermination() and log a warning if the result is negative, allowing to debug potential problem in the future. Otherwise, if we don't really care about awaiting termination, then maybe it's better to not call awaitTermination() at all. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FLINK-15838) Dangling CountDownLatch.await(timeout)
[ https://issues.apache.org/jira/browse/FLINK-15838?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17029179#comment-17029179 ] Roman Leventov commented on FLINK-15838: No > Dangling CountDownLatch.await(timeout) > -- > > Key: FLINK-15838 > URL: https://issues.apache.org/jira/browse/FLINK-15838 > Project: Flink > Issue Type: Bug > Components: Tests >Reporter: Roman Leventov >Priority: Major > > There are 16 occurrences in the codebase (all in test code) when the result > of CountDownLatch.await(timeout, TimeUnit) is not checked. It's like not > checking the result of File.delete(). The common fix is to wrap CDL.await() > call into assertTrue(). > All 16 places could be found using the following structural search in > IntelliJ: > $x$.await($y$, $z$); > With "CountDownLatch" type constraint on the $x$ variable. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FLINK-15838) Dangling CountDownLatch.await(timeout)
[ https://issues.apache.org/jira/browse/FLINK-15838?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17028463#comment-17028463 ] Roman Leventov commented on FLINK-15838: A typical outcome is a test not really testing production code, though falsely reporting "coverage". Such tests will be green regardless of whether the production logic is correct or not. > Dangling CountDownLatch.await(timeout) > -- > > Key: FLINK-15838 > URL: https://issues.apache.org/jira/browse/FLINK-15838 > Project: Flink > Issue Type: Bug >Reporter: Roman Leventov >Priority: Major > > There are 16 occurrences in the codebase (all in test code) when the result > of CountDownLatch.await(timeout, TimeUnit) is not checked. It's like not > checking the result of File.delete(). The common fix is to wrap CDL.await() > call into assertTrue(). > All 16 places could be found using the following structural search in > IntelliJ: > $x$.await($y$, $z$); > With "CountDownLatch" type constraint on the $x$ variable. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (FLINK-15838) Dangling CountDownLatch.await(timeout)
[ https://issues.apache.org/jira/browse/FLINK-15838?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17027563#comment-17027563 ] Roman Leventov commented on FLINK-15838: See also [https://github.com/code-review-checklists/java-concurrency#check-await], [https://youtrack.jetbrains.com/issue/IDEA-231640] > Dangling CountDownLatch.await(timeout) > -- > > Key: FLINK-15838 > URL: https://issues.apache.org/jira/browse/FLINK-15838 > Project: Flink > Issue Type: Bug >Reporter: Roman Leventov >Priority: Major > > There are 16 occurrences in the codebase (all in test code) when the result > of CountDownLatch.await(timeout, TimeUnit) is not checked. It's like not > checking the result of File.delete(). The common fix is to wrap CDL.await() > call into assertTrue(). > All 16 places could be found using the following structural search in > IntelliJ: > $x$.await($y$, $z$); > With "CountDownLatch" type constraint on the $x$ variable. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Created] (FLINK-15838) Dangling CountDownLatch.await(timeout)
Roman Leventov created FLINK-15838: -- Summary: Dangling CountDownLatch.await(timeout) Key: FLINK-15838 URL: https://issues.apache.org/jira/browse/FLINK-15838 Project: Flink Issue Type: Bug Reporter: Roman Leventov There are 16 occurrences in the codebase (all in test code) when the result of CountDownLatch.await(timeout, TimeUnit) is not checked. It's like not checking the result of File.delete(). The common fix is to wrap CDL.await() call into assertTrue(). All 16 places could be found using the following structural search in IntelliJ: $x$.await($y$, $z$); With "CountDownLatch" type constraint on the $x$ variable. -- This message was sent by Atlassian Jira (v8.3.4#803005)
