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.

Sign up for GitHub

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

Jump to bottom

storage/sources: Minor fixes for source table objects #29563

Merged
merged 3 commits into from
Sep 16, 2024
Merged

storage/sources: Minor fixes for source table objects #29563

merged 3 commits into from
Sep 16, 2024

Conversation

rjobanp
Copy link
Contributor

@rjobanp rjobanp commented Sep 16, 2024 •
edited
Loading

Motivation

Also fixes #29562 and fixes #29503 and fixes #29564

We were error logging whenever source tables were dropped after their corresponding source which was always happening when they are dropped together in a transaction due to the implicit ordering in src/adapter/src/coord/ddl.rs. There isn't an actual bug in dropping out-of-order so I could have just removed the error log but I thought that it would be better to keep it since it might be useful in any future issue around dependency ordering.

The error log happens here:

materialize/src/storage-controller/src/lib.rs

Lines 1453 to 1461 in 03b586b

// Adjust the source to remove this export.
let ingestion_state = match self.collections.get_mut(&ingestion_id) {
Some(ingestion_collection) => ingestion_collection,
// Primary ingestion already dropped.
None => {
tracing::error!("primary source {ingestion_id} seemingly dropped before subsource {id}");
continue;
}
};

Tips for reviewer

@MaterializeInc/testing I wasn't actually able to repro #29496 using the command in that issue but I believe this is the fix for that issue. If you could try to repro on this branch that would be awesome.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

@rjobanp rjobanp requested review from a team as code owners September 16, 2024 14:05
@rjobanp rjobanp mentioned this pull request Sep 16, 2024
Merged
@rjobanp rjobanp mentioned this pull request Sep 16, 2024
Closed
def-
def- reviewed Sep 16, 2024
View reviewed changes
Copy link
Contributor

@def- def- left a comment

Choose a reason for hiding this comment

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

This is failing a bunch, but is it possible to create a deterministic test for this?

@rjobanp
Copy link
Contributor Author

rjobanp commented Sep 16, 2024

This is failing a bunch, but is it possible to create a deterministic test for this?

@def- what do you mean by 'this'? Do you mean the primary source {ingestion_id} seemingly dropped before subsource {id} error report? Since it is only reported as an error log and is not an actual bug or error I don't know how we can add a test for it. It should be reproducible on main without this PR when doing a DROP SOURCE {} CASCADE for a source with a table attached, since that causes the source to be dropped from the storage controller before the table which is the thing I fixed here

@def-
Copy link
Contributor

def- commented Sep 16, 2024 •
edited
Loading

I meant a deterministic test for the issue that this PR fixes.

Something like this:

materialize/test/cluster/mzcompose.py

Lines 781 to 786 in 03b586b

# ensure that an error was put into the logs
c1 = c.invoke("logs", "clusterd1", capture=True)
assert (
"Net-zero records with non-zero accumulation in ReduceAccumulable"
in c1.stdout
)

@nrainer-materialize
Copy link
Contributor

Something like this:

materialize/test/cluster/mzcompose.py

Lines 781 to 786 in 03b586b

# ensure that an error was put into the logs
c1 = c.invoke("logs", "clusterd1", capture=True)
assert (
"Net-zero records with non-zero accumulation in ReduceAccumulable"
in c1.stdout
)

Well, we are expecting that this error message no longer occurs.

@rjobanp
Copy link
Contributor Author

rjobanp commented Sep 16, 2024

Yes exactly, if this fix works we will no longer see other tests fail when encountering that error message

@rjobanp rjobanp mentioned this pull request Sep 16, 2024
Merged
@rjobanp rjobanp merged commit f5a61f5 into MaterializeInc:main Sep 16, 2024
84 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 16, 2024
@rjobanp rjobanp deleted the source-table-fixes-1 branch September 16, 2024 15:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@def- def- def- left review comments

@jkosh44 jkosh44 jkosh44 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@rjobanp @def- @nrainer-materialize @jkosh44