Skip to content

fix(storage): BidiAppendableUpload Takeover operation fixes#13776

Open
Dhriti07 wants to merge 2 commits into
mainfrom
takoever_fixes
Open

fix(storage): BidiAppendableUpload Takeover operation fixes#13776
Dhriti07 wants to merge 2 commits into
mainfrom
takoever_fixes

Conversation

@Dhriti07

@Dhriti07 Dhriti07 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

BidiAppendableUpload Takeover operation fixes

@Dhriti07 Dhriti07 requested review from a team as code owners July 15, 2026 09:23
@Dhriti07 Dhriti07 changed the title BidiAppendableUpload Takoever operation fixes fix(storage): BidiAppendableUpload Takoever operation fixes Jul 15, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates state transitions and retry logic in BidiUploadState.java, and enables the PROD backend for ITAppendableUploadTest. It also refactors the awaitState and awaitAck methods to move the await call into the loop body. The feedback suggests increasing the short 5ms timeout in these loops to mitigate potential high CPU usage and performance degradation caused by frequent polling.

Comment on lines +893 to 898
while (!states.contains(this.state)) {
if (resultFuture.isDone()) {
return;
}
stateUpdated.await(5, TimeUnit.MILLISECONDS);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using a very short timeout of 5 milliseconds for await in a loop results in frequent waking up and lock acquisition (polling), which can lead to high CPU usage and performance degradation under load.

If resultFuture completion signals stateUpdated (or if you can add a listener to resultFuture to signal it upon completion), you can use a simple, indefinite stateUpdated.await() without a timeout. If a timeout is absolutely necessary as a fallback, consider increasing it to a more reasonable value (e.g., 100ms or 1s) to reduce CPU overhead.

Suggested change
while (!states.contains(this.state)) {
if (resultFuture.isDone()) {
return;
}
stateUpdated.await(5, TimeUnit.MILLISECONDS);
}
while (!states.contains(this.state)) {
if (resultFuture.isDone()) {
return;
}
stateUpdated.await(100, TimeUnit.MILLISECONDS);
}

Comment on lines +920 to 925
while (confirmedBytes < writeOffset) {
if (resultFuture.isDone()) {
return;
}
confirmedBytesUpdated.await(5, TimeUnit.MILLISECONDS);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using a very short timeout of 5 milliseconds for await in a loop results in frequent waking up and lock acquisition (polling), which can lead to high CPU usage and performance degradation under load.

If resultFuture completion signals confirmedBytesUpdated (or if you can add a listener to resultFuture to signal it upon completion), you can use a simple, indefinite confirmedBytesUpdated.await() without a timeout. If a timeout is absolutely necessary as a fallback, consider increasing it to a more reasonable value (e.g., 100ms or 1s) to reduce CPU overhead.

Suggested change
while (confirmedBytes < writeOffset) {
if (resultFuture.isDone()) {
return;
}
confirmedBytesUpdated.await(5, TimeUnit.MILLISECONDS);
}
while (confirmedBytes < writeOffset) {
if (resultFuture.isDone()) {
return;
}
confirmedBytesUpdated.await(100, TimeUnit.MILLISECONDS);
}

@@ -544,6 +544,7 @@ final void updateStateFromResponse(BidiWriteObjectResponse response) {
if (state == State.INITIALIZING || state == State.RETRYING) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: Fix typo in PR message.

@Dhriti07 Dhriti07 changed the title fix(storage): BidiAppendableUpload Takoever operation fixes fix(storage): BidiAppendableUpload Takeover operation fixes Jul 16, 2026
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