[FLINK-4218] [checkpoints] Do not rely on FileSystem to determine state sizes #2544
Conversation
|
@StefanRRichter Maybe interesting for you to review. |
…te sizes This prevents failures on eventually consistent S3, where the operations for keys (=entries in the parent directory/bucket) are not guaranteed to be immediately consistent (visible) after a blob was written.
StephanEwen
force-pushed
the
state_size_fix
branch
from
7ce2de7
to
dc12d0b
Compare
| @@ -301,9 +301,16 @@ public StreamStateHandle closeAndGetHandle() throws IOException { | |||
| } | |||
| else { | |||
| flush(); | |||
|
|
|||
| long size = -1; | |||
There was a problem hiding this comment.
I am not sure if returning -1 as size on exception is ideal. Currently, this value should one be used in the calculation of meta data, but one might be tempted to use it e.g. to preallocate a byte[] to read the file into, so this should at least be documented in StateObject. Furthermore, we make the assumption that the stream position is also equal to the final file size. Not entirely sure if this holds for all streams and file systems, but I guess this is the best we can do without asking the file system for meta data.
There was a problem hiding this comment.
Stream position should be okay to determine the state size. All instances I checked were accurate there.
Also, given that the size is more informational and should not be relied upon, it should be all the less critical.
As
| */ | ||
| long getStateSize() throws Exception; | ||
| long getStateSize() throws IOException; |
There was a problem hiding this comment.
I think with the change in StreamStateHandle, even throwing IOException becomes obsolete now for all existing implementations. We might remove it.
There was a problem hiding this comment.
I think we should do that.
|
Manually merged in 95e9004 |
This prevents failures on eventually consistent S3, where the operations for keys (=entries in the parent directory/bucket) are not guaranteed to be immediately consistent (visible) after a blob was written.
Not relying on any operation on keys (= requesting
FileStatus) should mitigate the problem.This also changes the exception signature from
getStateSize()fromExceptiontoIOException, which fits more natural with the exception signatures of some other I/O methods.Related issue: We may still want to have retries on
FileStatusoperations on S3, for other parts of the system (like FileOutputFormats)