[FLINK-30389] Add retry to read hints #453
Conversation
| return Long.parseLong(FileUtils.readFileUtf8(path)); | ||
| while (retryNumber++ < READ_HINT_RETRY_NUM) { | ||
| if (path.getFileSystem().exists(path)) { | ||
| return Long.parseLong(FileUtils.readFileUtf8(path)); |
There was a problem hiding this comment.
We need to catch exception and ignore exception here.
There was a problem hiding this comment.
int retryNumber = 0;
while (retryNumber++ < READ_HINT_RETRY_NUM) {
try {
return Long.parseLong(FileUtils.readFileUtf8(path));
} catch (Exception ignored) {
}
TimeUnit.MILLISECONDS.sleep(READ_HINT_RETRY_INTERVAL);
}
We should skip inner exception, then we can retry to read.
There was a problem hiding this comment.
OK. I've added a fix-up commit. Currently only exception of "Long.parseLong(FileUtils.readFileUtf8(path))" is catched, and others are thrown to invoker.
There was a problem hiding this comment.
Maybe we can just remove if (path.getFileSystem().exists(path)) {, we already have catch the exception for FileUtils.readFileUtf8(path).
| public static final String EARLIEST = "EARLIEST"; | ||
| public static final String LATEST = "LATEST"; | ||
| private static final int READ_HINT_RETRY_NUM = 10; | ||
| private static final int READ_HINT_RETRY_INTERVAL = 200; |
There was a problem hiding this comment.
Maybe the better choice is:
READ_HINT_RETRY_NUMis 3.READ_HINT_RETRY_INTERVALis 1ms.
| } catch (Exception e) { | ||
| LOG.info( | ||
| "Failed to read hint file " + fileName + ". Falling back to listing files.", e); | ||
| TimeUnit.MILLISECONDS.sleep(READ_HINT_RETRY_INTERVAL); |
There was a problem hiding this comment.
Catch and ignore the InterruptedException
| while (retryNumber++ < READ_HINT_RETRY_NUM) { | ||
| try { | ||
| return Long.parseLong(FileUtils.readFileUtf8(path)); | ||
| } catch (IOException ignored) { |
There was a problem hiding this comment.
Exception is more safe, after all, read hint is just an optimization
JingsongLi
left a comment
JingsongLi
left a comment
There was a problem hiding this comment.
Hi @WencongLiu thanks for the update, just one comment.
JingsongLi
left a comment
JingsongLi
left a comment
There was a problem hiding this comment.
Looks good to me! Thanks @WencongLiu
2 participants
What is the purpose of the change
Add retry to read hints in SnapshotManager
Brief change log
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation