You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by sihuazhou <gi...@git.apache.org> on 2018/05/16 07:51:11 UTC
[GitHub] flink pull request #6020: [FLINK-9373][state] Always call RocksIterator.stat...
GitHub user sihuazhou opened a pull request:
https://github.com/apache/flink/pull/6020
[FLINK-9373][state] Always call RocksIterator.status() to check the internal error of RocksDB
## What is the purpose of the change
Currently, when using RocksIterator we only use the `iterator.isValid()` to check whether we have reached the end of the iterator. But that is not enough, if we refer to RocksDB's wiki https://github.com/facebook/rocksdb/wiki/Iterator#error-handling we should find that even if `iterator.isValid()=true`, there may also exist some internal errors. A safer way to use the `RocksIterator` is to always call the `iterator.status()` to check the internal error of RocksDB. There is one case from user email seems to lost data because of this http://apache-flink-user-mailing-list-archive.2336050.n4.nabble.com/Missing-MapState-when-Timer-fires-after-restored-state-td20134.html (I'm not so sure yet)
## Brief change log
- *Always call RocksIterator.status() to check the internal error of RocksDB*
## 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)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sihuazhou/flink FLINK-9373
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/6020.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 #6020
----
commit b1d531949d521bb7d72cd389dfe72a4a5cf1bfc9
Author: sihuazhou <su...@...>
Date: 2018-05-16T07:47:05Z
Always call RocksIterator.status() to check the internal error of RocksDB
----
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/6020
Also from the RocksDB docs: `In another word, if Iterator::Valid() is true, status() is guaranteed to be OK()`
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:
https://github.com/apache/flink/pull/6020
Agree, should be correct first before fast! Could you please have a look at this? I think it's already for a look now~
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:
https://github.com/apache/flink/pull/6020
cc @StefanRRichter
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/6020
Maybe we should ask them on their issue tracker what the best practise is? I cannot remember seeing such checks in their code examples. Have a hard time to believe that this can be true, because it is not really documented on the Java API and also why wouldn't they always call `status` internally?
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:
https://github.com/apache/flink/pull/6020
I'm going to check the native implementation and see whether the `status()` is a super cheap option...
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:
https://github.com/apache/flink/pull/6020
Oh My God...Is that means we need to wrap the `RocksIterator` to delegate all it API?
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/6020
Yes, but eventually it will also return `false`, which is essentially the same as waiting until the loop terminates. Anyways, I think after the loop is the nicer way.
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:
https://github.com/apache/flink/pull/6020
The reasons that the travis given red light is unrelated...
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:
https://github.com/apache/flink/pull/6020
FYI, I found this issue related to problem: https://github.com/facebook/rocksdb/issues/3558
---
[GitHub] flink pull request #6020: [FLINK-9373][state] Fix potential data losing for ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/6020
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/6020
Thanks @sihuazhou ! LGTM 👍 Will merge this.
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/6020
Let's wait a bit more for their response. It seems like this example is older than their corrected docs.
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:
https://github.com/apache/flink/pull/6020
cc @StefanRRichter
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/6020
After double checking with the RocksDB docs, I am afraid that we need to introduce more checks because for example the point out that also after methods like `seek` the iterator an become corrupted. And if the status flag is potentially cleared, that means we need to check in all the places...crazy :-(
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:
https://github.com/apache/flink/pull/6020
That is a good question, and I'm not sure...but I think that seems to be...
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:
https://github.com/apache/flink/pull/6020
Agreed!
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/6020
A quick general question: could you observe any performance impact from calling the `status()` method in the loops. It looks like a native method and I am not sure that it is inexpensive. Maybe the better idea is to only check `isValid()` in the loops and check `status()` only once after the loop to ensure that everything was well and complete. Maybe that is also the reason why this is split into two methods in the first place. What do you think?
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:
https://github.com/apache/flink/pull/6020
I think I am a bit torn here now...
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:
https://github.com/apache/flink/pull/6020
@StefanRRichter NO, I think that couldn't fix this issue, the problem here is that even `iterator.isValid()` return `true`, there may also some internal error in RocksDB. What do you think?
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/6020
Oh you are right, this is confusing :-) So does this also mean the status flag is cleared when we simple continue iterating and only check in the end?
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/6020
You could also in `isRocksIteratorValid` run the check only if the return value is `false` if you like the helper method to avoid people forgetting about this check.
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:
https://github.com/apache/flink/pull/6020
@StefanRRichter I had a look at the implementation of the iterators in RocksDB, I found status just return the flag first `_status` as the result without any complex computation, But for some `composite Iterator` like the `MergeIteraor` and `TwoLevelIterator` it need to check all the `InternalIterator` they hold to decide the final status, and I also found the iterator could be reset to `OK` in some cases...Hmm...do you think this is super cheap or not?
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/6020
It depends, maybe this is already covered currently because we might always do an iteration attempt that checks right after the seek. But in general, this is not very nice and fragile if true.
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:
https://github.com/apache/flink/pull/6020
Have't received any response from RocksDB yet, but I found this example with using `RocksIterator#stats()`: https://github.com/facebook/rocksdb/blob/3453870677ee2648f38d70fe8aa7fa16a93a96d2/java/samples/src/main/java/RocksDBSample.java
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:
https://github.com/apache/flink/pull/6020
@StefanRRichter No, I didn't have any performance tests yet. I think you are right! Your proposal is the way I'm going to choose. Addressing this...
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on the issue:
https://github.com/apache/flink/pull/6020
I think that is the incorrect one, If I'm not confused by the wiki's content...
---
[GitHub] flink issue #6020: [FLINK-9373][state] Fix potential data losing for RocksDB...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/6020
It sounds cheap if they just `&` all the flags from the sub iterators. In the end, we can see if there is a performance drop but better be correct first before fast.
---