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

[BEAM-14181] Make sure to evict connections from cache after closing them #17187

Merged
merged 2 commits into from Mar 28, 2022

Conversation

reuvenlax
Copy link
Contributor

No description provided.

@aaltay
Copy link
Member

aaltay commented Mar 25, 2022

@youngoli - Could we consider this for cherry picking into the release branch?

@aaltay aaltay changed the title Fix cache behavior [BEAM-14181] Fix cache behavior Mar 25, 2022
@reuvenlax reuvenlax changed the title [BEAM-14181] Fix cache behavior [BEAM-14181] Make sure to evict connections from cache after closing thiem Mar 25, 2022
@reuvenlax reuvenlax requested a review from pabloem March 25, 2022 21:34
@@ -173,6 +173,12 @@ public DestinationState(
this.useDefaultStream = useDefaultStream;
}

void teardown() {
if (streamAppendClient != null) {
runAsyncIgnoreFailure(closeWriterExecutor, streamAppendClient::unpin);
Copy link
Member

Choose a reason for hiding this comment

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

do we need to invalidate it from the cache as well here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No - unpin will only close the connection if close() has already been called. Since the client is still healthy, we can leave it in the cache

@pabloem
Copy link
Member

pabloem commented Mar 25, 2022

LGTM otherwise

@reuvenlax reuvenlax changed the title [BEAM-14181] Make sure to evict connections from cache after closing thiem [BEAM-14181] Make sure to evict connections from cache after closing them Mar 25, 2022
@youngoli
Copy link
Contributor

A cherry-pick shouldn't be a problem. I'm currently waiting on some other release-blocking issues so I can cherry-pick this in.

Can someone add more information to the Jira to make it explicit why this is a release-blocking issue (especially identifying when the issue was introduced and describing what problems it's causing)?

@reuvenlax
Copy link
Contributor Author

@youngoli Yes - roughly this causes the BigQuery sink to sometimes get full stuck and never recover, and the pipeline grinds to a halt. I believe the regression was introduced in the last Beam release.

@reuvenlax
Copy link
Contributor Author

Run Java Precommit

@reuvenlax
Copy link
Contributor Author

Run Java PreCommit

@reuvenlax reuvenlax merged commit e7d7525 into apache:master Mar 28, 2022
youngoli pushed a commit to youngoli/beam that referenced this pull request Mar 29, 2022
…ections from cache after closing them

(cherry picked from commit e7d7525)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants