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

Cherrypick lock backoff retry to 3.9 #18485

Merged
merged 23 commits into from
Jan 21, 2025
Merged

Conversation

aliehsaeedii
Copy link
Contributor

This PR cherry-picks the commits of this and this PR to 3.9.
The above-listed two PRs aim at implementing exponential back off retry for state directory lock to increase the time between two consecutive attempts of acquiring the lock.

Copy link
Member

@cadonna cadonna left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @aliehsaeedii !

I have one question.

Additionally, checkstyle is failing due to an unused import.

@@ -4578,48 +4626,6 @@ public void shouldNotFailForTimeoutExceptionOnConsumerCommit() {
verify(consumer, times(2)).commitSync(any(Map.class));
}

@Test
public void shouldNotFailForTimeoutExceptionOnCommitWithEosAlpha() {
Copy link
Member

Choose a reason for hiding this comment

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

Why do you remove this test?

Copy link
Member

@cadonna cadonna left a comment

Choose a reason for hiding this comment

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

Thanks for the update, @aliehsaeedii !

I have one minor comment.

@@ -364,7 +364,6 @@ synchronized boolean lock(final TaskId taskId) {
} else {
lockedTasksToOwner.put(taskId, Thread.currentThread());
// make sure the task directory actually exists, and create it if not
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that this comment does not make sense anymore since the following line is removed. This also applies to trunk. Could you please remove the comment here and open a PR to remove the comment for trunk?

Copy link
Member

@cadonna cadonna left a comment

Choose a reason for hiding this comment

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

Thanks for the update, @aliehsaeedii!

LGTM!

@cadonna cadonna merged commit 310867a into apache:3.9 Jan 21, 2025
1 check failed
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.

2 participants