Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] Retry if snapshot commit hint failed. #4701

Merged
merged 4 commits into from
Dec 13, 2024

Conversation

leaves12138
Copy link
Contributor

@leaves12138 leaves12138 commented Dec 13, 2024

Purpose

Maybe over-writer file will fail, so it's better to add some retries.

Tests

API and Format

Documentation

@@ -44,4 +44,19 @@ public static FileIOLoader checkAccess(FileIOLoader fileIO, Path path, CatalogCo
io.exists(path);
return fileIO;
}

public static void overwriteFileUtf8WithRetry(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inline this in commitHint

fileIO.overwriteFileUtf8(path, content);
return;
} catch (IOException e) {
if (loopTime == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sleep a random value here?

@@ -883,6 +884,6 @@ public void commitEarliestHint(long snapshotId) throws IOException {

private void commitHint(long snapshotId, String fileName, Path dir) throws IOException {
Path hintFile = new Path(dir, fileName);
fileIO.overwriteFileUtf8(hintFile, String.valueOf(snapshotId));
FileIOUtils.overwriteFileUtf8WithRetry(fileIO, hintFile, String.valueOf(snapshotId), 3);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard code here? Or make it configurable better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

return;
} catch (IOException e) {
try {
Thread.sleep(RANDOM.nextInt(1000) + 500);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ThreadLocalRandom.current()

Thread.sleep(RANDOM.nextInt(1000) + 500);
} catch (InterruptedException ex) {
// throw root cause
throw new RuntimeException(e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thread.currentThread().interrupt();

Copy link
Contributor

@JingsongLi JingsongLi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@JingsongLi JingsongLi merged commit a263170 into apache:master Dec 13, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants