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

execinfra: remove MetadataTest* processors #57838

Merged
merged 1 commit into from
Jun 29, 2022

Conversation

yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented Dec 11, 2020

This commit removes MetadataTestSender and MetadataTestReceiver
processors since they no longer provide much value. I believe they were
introduced when we added a ProducerMetadata as a return parameter to
Next method in order to ensure that at least some artificial metadata
is propagated correctly throughout the whole flow.

The main goal of this commit is the removal of fakedist-metadata and
5node-metadata logic test configs in order to speed up the CI time.

The justification for removal of these processors without putting anything
in their place is that these processors are not that useful - the only
thing they can test is that some metadata is propagated through the
row-based flows. Note that they don't test whether all necessary
metadata is emitted (for example, whether LeafTxnFinalState). We've
been using the vectorized engine as the default for a while now, and these
processors don't get planned with the vectorized flows. Thus, it seems
silly to have a logic test config like fakedist-metadata that is part
of the default configs.

Addresses: #57268.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich yuzefovich requested review from asubiotto and a team December 11, 2020 22:30
@asubiotto
Copy link
Contributor

What's the motivation for removing this now rather than waiting until we have the testing you mention in your comment?

@yuzefovich
Copy link
Member Author

The motivation is that these processors are not that useful - the only thing they can test is that some metadata is propagated through the row-based flow, note that they don't test whether all necessary metadata is emitted (for example, whether LeafTxnFinalState). I think originally these processors were introduced to make sure that processors don't swallow the received metadata and, instead, propagate it further.

I believe having the testing framework I mentioned for the vectorized engine and having these processors are orthogonal issues since they would work in different scenarios, so the question is whether it is ok to remove these processors at all without introducing some other testing mechanism for the row-based flow and when it is ok to do so.

I think it is ok to remove these processors without putting anything in their place since they already don't provide much value, but it is not urgent to do so, so we can keep this PR in the backlog for now if you prefer.

@yuzefovich
Copy link
Member Author

Ok, let's put it on the backlog for now.

@yuzefovich yuzefovich added the do-not-merge bors won't merge a PR with this label. label Dec 15, 2020
@tbg tbg added the X-noremind Bots won't notify about PRs with X-noremind label May 6, 2021
This commit removes `MetadataTestSender` and `MetadataTestReceiver`
processors since they no longer provide much value. I believe they were
introduced when we added a `ProducerMetadata` as a return parameter to
`Next` method in order to ensure that at least some artificial metadata
is propagated correctly throughout the whole flow.

The main goal of this commit is the removal of `fakedist-metadata` and
`5node-metadata` logic test configs in order to speed up the CI time.

The justification for removal of these processors without putting
in their place is that these processors are not that useful - the only
thing they can test is that *some* metadata is propagated through the
row-based flows. Note that they don't test whether all necessary
metadata is emitted (for example, whether `LeafTxnFinalState`). We've
using the vectorized engine as the default for a while now, and these
processors don't get planned with the vectorized flows. Thus, it seems
silly to have a logic test config like `fakedist-metadata` that is part
of the default configs.

Release note: None
@yuzefovich yuzefovich requested a review from a team as a code owner June 24, 2022 18:04
@yuzefovich yuzefovich removed do-not-merge bors won't merge a PR with this label. X-noremind Bots won't notify about PRs with X-noremind labels Jun 24, 2022
@yuzefovich yuzefovich removed the request for review from asubiotto June 24, 2022 18:04
@yuzefovich yuzefovich marked this pull request as draft June 24, 2022 18:06
@yuzefovich yuzefovich marked this pull request as ready for review June 24, 2022 20:07
@yuzefovich yuzefovich requested review from mgartner and michae2 June 24, 2022 20:07
@yuzefovich
Copy link
Member Author

I revived this PR and would like to push it over the line. Previously, there was a pushback from Alfonso that we shouldn't remove testing infra without putting anything in place, but I'm arguing that the existing testing infra that is being removed here has essentially zero value:

The justification for removal of these processors without putting anything
in their place is that these processors are not that useful - the only
thing they can test is that some metadata is propagated through the
row-based flows. Note that they don't test whether all necessary
metadata is emitted (for example, whether LeafTxnFinalState). We've
been using the vectorized engine as the default for a while now, and these
processors don't get planned with the vectorized flows. Thus, it seems
silly to have a logic test config like fakedist-metadata that is part
of the default configs.

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

@yuzefovich
Copy link
Member Author

TFTR!

bors r+

@craig craig bot merged commit 0917fdc into cockroachdb:master Jun 29, 2022
@craig
Copy link
Contributor

craig bot commented Jun 29, 2022

Build succeeded:

@yuzefovich yuzefovich deleted the remove-meta branch June 29, 2022 19:41
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.

5 participants