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

tm state: don't populate metadata in updateLocked #8362

Merged
merged 1 commit into from
Jun 22, 2021

Conversation

deepthi
Copy link
Member

@deepthi deepthi commented Jun 22, 2021

Description

This is a partial revert of #8107. Calling populateMetadata in updateLocked causes publishState to fail when semi-sync is enabled.

Related Issue(s)

Fixes #8505

Checklist

  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

…d during reparents and hangs when semi-sync is enabled

Signed-off-by: deepthi <deepthi@planetscale.com>
@deepthi deepthi requested review from ajm188, aquarapid and sougou and removed request for shlomi-noach June 22, 2021 04:21
@deepthi
Copy link
Member Author

deepthi commented Jun 22, 2021

@ajm188 it's possible that the right fix is as simple as changing these conditions:
https://github.com/vitessio/vitess/pull/8107/files#diff-8b21ba72e9fc7850bc4516d90ebf0639ac39fbef7e7b0ac534e2f470574c5a77R290-R297

	if ts.tm.MetadataManager == nil {
		return
	}

	if ts.isOpening && !*initPopulateMetadata {
		return
	}

During PRS isOpening is false and the code gets executed even when initPopulateMetadata is false. I believe the check really needs to be

	if ts.tm.MetadataManager == nil || !*initPopulateMetadata {
		return
	}

@deepthi
Copy link
Member Author

deepthi commented Jun 22, 2021

@ajm188 it's possible that the right fix is as simple as changing these conditions:
https://github.com/vitessio/vitess/pull/8107/files#diff-8b21ba72e9fc7850bc4516d90ebf0639ac39fbef7e7b0ac534e2f470574c5a77R290-R297

On second thought, populateMetadata should NEVER be called in a code path that runs during a reparent.
No DB writes can happen during that process because they will hang when semi-sync is on.

@ajm188
Copy link
Contributor

ajm188 commented Jun 22, 2021

I am thinking we should add a flag to the section of ChangeTabletType here, and then only call populateMetadata if we're not transition from/to MASTER

@deepthi
Copy link
Member Author

deepthi commented Jun 22, 2021

I'm merging this for now to fix the regression. It will need to be fixed properly later (with appropriate tests).

@deepthi deepthi merged commit 7f3da8e into vitessio:main Jun 22, 2021
@deepthi deepthi deleted the ds-tm-update branch June 22, 2021 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants