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

descmetadata: do not use experimental setting #87288

Merged
merged 1 commit into from
Sep 1, 2022

Conversation

yuzefovich
Copy link
Member

Release justification: low-risk cleanup.

Release note: None

Release justification: low-risk cleanup.

Release note: None
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich yuzefovich marked this pull request as ready for review September 1, 2022 18:54
@yuzefovich yuzefovich requested review from fqazi and ajwerner September 1, 2022 18:54
@yuzefovich
Copy link
Member Author

I don't quite understand why this was introduced in #83592 - am I missing something? The CI seems happy.

@fqazi
Copy link
Collaborator

fqazi commented Sep 1, 2022

The concern was if ExperimentalDistSQLPlanningAlways was somehow set inside the session, anything inheriting the session run with that. That could cause internal inserts from the metadata updater, etc.. to fail. This used to be a problem on logictest at one point

@yuzefovich
Copy link
Member Author

I see. This would only be applicable to a couple of logic test files (that explicitly do SET experimental_distsql_planning = always), so I think it's not good that we create a copy of the session data in all cases, production including. I briefly stressed the relevant files with this PR and didn't see any flakes, so I think we should proceed with this change. If the flakes do happen, then we probably will need to add RESET experimental_distsql_planning to the end of those files.

Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

@yuzefovich
Copy link
Member Author

TFTR!

bors r+

Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

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

I'm okay with that approach, the onus should be on the tests themselves. We used to have a logictest mode that forced it in other cases I think

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

@yuzefovich
Copy link
Member Author

We did have -spec-planning configs (which were removed like a month ago), but they were using On and not Always for this experimental setting.

@craig
Copy link
Contributor

craig bot commented Sep 1, 2022

Build succeeded:

@craig craig bot merged commit 92102da into cockroachdb:master Sep 1, 2022
@yuzefovich yuzefovich deleted the metadata-updater branch September 1, 2022 23:00
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.

3 participants