Skip to content

Fix empty stream name encountered in StorageApiFinalizeWrtiesDoFn#38410

Draft
Abacn wants to merge 1 commit into
apache:masterfrom
Abacn:draft-fix-empty-stream-name
Draft

Fix empty stream name encountered in StorageApiFinalizeWrtiesDoFn#38410
Abacn wants to merge 1 commit into
apache:masterfrom
Abacn:draft-fix-empty-stream-name

Conversation

@Abacn
Copy link
Copy Markdown
Contributor

@Abacn Abacn commented May 7, 2026

Follow up #38264

Please add a meaningful description for your change here


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

@reuvenlax
Copy link
Copy Markdown
Contributor

I don't really understand this PR - can you explain it? The initializeContext/clearContexts structure is the one that has been there for a very long time, so I'm not sure why you're changing it.

}

// Re-establish the client with the new stream.
clearClients.accept(failedContexts);
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.

clearClients no longer even looks at failedContexts. We should simplify it and remove the parameter

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.

We should probably rename clearClients to resetClient

@Abacn
Copy link
Copy Markdown
Contributor Author

Abacn commented May 13, 2026

I don't really understand this PR - can you explain it? The initializeContext/clearContexts structure is the one that has been there for a very long time, so I'm not sure why you're changing it.

Yeah agree that this part sounds less relevant. This is just an AI-assistent attempt and it doesn't look clear to me either. It's just for reference.

The part that moves streamName.write(""); around looks a bit more plausible. However in any case we'd need way (and time) to test it. In terms of release schedule would it be preferred to rollback (#38479) then roll forward to get a clean commit at release cut?

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.

2 participants