Skip to content

Conversation

@youngoli
Copy link
Contributor


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

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • 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

See CI.md for more information about GitHub Actions CI.

@youngoli
Copy link
Contributor Author

R: @lukecwik @dpcollins-google @TheNeuralBit (whoever gets to it first)

For now I'm rolling these back to give the RC a try.

@dpcollins-google
Copy link
Contributor

For #17004: the issue mentioned by @lukecwik was not reproducible, and I think we should import again
For #17094: This revealed an issue somewhere else in the codebase, what is the triggering test? I'll run it and fix that location as well

@dpcollins-google
Copy link
Contributor

#17228 fixes the metrics issue

@TheNeuralBit
Copy link
Member

Shouldn't we roll these back on master and then cherrypick to the release branch?

@dpcollins-google
Copy link
Contributor

I don't think either of these needs to be rolled back on master or the release branch. I think the issue with #17004 was a false positive (or at least not reproducible) and #17094 just revealed an issue that would otherwise have failed later in processing on runnerv2

@dpcollins-google
Copy link
Contributor

#17228 is merged.

@youngoli
Copy link
Contributor Author

youngoli commented Apr 1, 2022

After some offline investigation, we were able to reproduce the error. As for cherry-picking #17228, I'm going forward with rollback because I'm hesitant to delay any further for a cherry-pick (I already should've started the RC yesterday).

@youngoli
Copy link
Contributor Author

youngoli commented Apr 1, 2022

Nevermind, I finally understood what people have been trying to explain about #17094 which is that it only reveals a bug that would still be present without it. So rolling back the PR wouldn't fix anything, a cherry-pick is actually necessary.

I'll change this PR to only roll back #17004 and create a new cherry-pick PR.

@TheNeuralBit
Copy link
Member

@youngoli you said earlier:

I think the issue with #17004 was a false positive (or at least not reproducible)

If it was a false positive do we need to roll it back on the release branch?

@youngoli
Copy link
Contributor Author

youngoli commented Apr 1, 2022

That was @dpcollins-google btw. But at the time he wasn't able to reproduce it and was saying the error we originally got was the false positive (i.e. an error when there wasn't actually a problem).

But like I said, we investigated it offline (Daniel, Luke, and I) and we were able to replicate it after all. So it does look like a rollback on master may be necessary too.

@youngoli youngoli changed the title [rollback][release-2.38.0][BEAM-14116][BEAM-14179] Rolling back PRs causing release-blocking issues. [rollback][release-2.38.0][BEAM-14116] Rollback of #17004: Chunk commit requests dynamically Apr 1, 2022
@youngoli youngoli merged commit ede9921 into apache:release-2.38.0 Apr 1, 2022
@dpcollins-google
Copy link
Contributor

@TheNeuralBit it was a flaky failure, so I wasn't able to reproduce it but it replicated like 50/100 runs

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.

3 participants