You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by yzandrew <gi...@git.apache.org> on 2018/06/18 18:54:25 UTC
[GitHub] flink pull request #6180: [FLINK-9524][Table API & SQL] check whether a clea...
GitHub user yzandrew opened a pull request:
https://github.com/apache/flink/pull/6180
[FLINK-9524][Table API & SQL] check whether a clean-up timer is expeired in ProcTimeBoundedRangeOver…
… to prevent NPE
## What is the purpose of the change
This pull request solve the bug that causes NPE in ProcTimeBoundedRangeOver. Now the onTimer method will check whether the timer is a expired clean-up timer.
## Brief change log
- ProcTimeBoundedRangeOver check whether the timer is a expired clean-up timer. It's done by a null check against the elements within rowMapState. If it's null, it means the timer is an expired clean-up timer and bypass the remained logic.
## 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): 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? not applicable
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/yzandrew/flink master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/6180.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 #6180
----
commit 719bdebf6bdb0e3e3f9aa0acd0bb98040a259a59
Author: yan.zhou <yz...@...>
Date: 2018-06-18T18:42:29Z
[FLINK-9524] check whether a clean-up timer is expeired in ProcTimeBoundedRangeOver to prevent NPE
----
---
[GitHub] flink pull request #6180: [FLINK-9524][Table API & SQL] check whether a clea...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/6180#discussion_r197453235
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/runtime/aggregate/ProcTimeBoundedRangeOver.scala ---
@@ -132,74 +132,80 @@ class ProcTimeBoundedRangeOver(
val currentTime = timestamp - 1
var i = 0
+ // get the list of elements of current proctime
+ val currentElements = rowMapState.get(currentTime)
- // initialize the accumulators
- var accumulators = accumulatorState.value()
+ // clean-up timers might expire, which pass the needToCleanupState check above.
+ // null-check is necessary here, otherwise NPE might be thrown.
+ if(null != currentElements) {
--- End diff --
How about changing this to
```
if (null == currentElements) {
return
}
```
It would be touch much fewer lines of code and IMO easier to read (less nesting).
Please note the space between `if` and the condition.
---
[GitHub] flink issue #6180: [FLINK-9524][Table API & SQL] check whether a clean-up ti...
Posted by yzandrew <gi...@git.apache.org>.
Github user yzandrew commented on the issue:
https://github.com/apache/flink/pull/6180
Hi,
I have updated the pull request based on your comments. In order to have fewer lines of changes, I squashed the commits.
---
[GitHub] flink pull request #6180: [FLINK-9524][Table API & SQL] check whether a clea...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/6180
---
[GitHub] flink pull request #6180: [FLINK-9524][Table API & SQL] check whether a clea...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/6180#discussion_r197453796
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/runtime/aggregate/ProcTimeBoundedRangeOver.scala ---
@@ -132,74 +132,80 @@ class ProcTimeBoundedRangeOver(
val currentTime = timestamp - 1
var i = 0
+ // get the list of elements of current proctime
+ val currentElements = rowMapState.get(currentTime)
- // initialize the accumulators
- var accumulators = accumulatorState.value()
+ // clean-up timers might expire, which pass the needToCleanupState check above.
--- End diff --
Please rephrase comment to
```
// Expired clean-up timers pass the needToCleanupState() check.
// Perform a null check to verify that we have data to process.
```
---
[GitHub] flink issue #6180: [FLINK-9524][Table API & SQL] check whether a clean-up ti...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/6180
Thanks for the update @yzandrew.
Merging
---