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

coord: fix propagation of negative multiplicities error #2316

Merged
merged 1 commit into from Mar 15, 2020

Conversation

@benesch
Copy link
Member

benesch commented Mar 15, 2020

The coordinator was inadvertently converting this error into a
PeekResponse::Canceled. Looks to be a missed code path from #1648.

Fix #2128.
Fix #2314.

The coordinator was inadvertently converting this error into a
PeekResponse::Canceled. Looks to be a missed code path for #1648.

Fix #2128.
Fix #2314.
@benesch benesch requested a review from frankmcsherry Mar 15, 2020
Copy link
Member

frankmcsherry left a comment

This looks great! I'm a bit confused about the test. I thought the error would be

    return Result::Err(format!(
        "Negative multiplicity: {} for {:?}",
        copies,
        row.unpack(),
    ));

Does the data get lost somewhere along the way? It's all good in either case, just trying to make sure I understand other moments where info might get lost.

@benesch

This comment has been minimized.

Copy link
Member Author

benesch commented Mar 15, 2020

Testdrive tests errors by doing a string contains—so the full error is as you expect. Didn't want to hardcode the debug impl of the datum into the test file.

@benesch benesch merged commit f087884 into MaterializeInc:master Mar 15, 2020
14 checks passed
14 checks passed
buildkite/tests Build #5939 passed (24 minutes, 12 seconds)
Details
buildkite/tests/bath-lint-and-rustfmt Passed (1 minute, 9 seconds)
Details
buildkite/tests/bulb-bulb-full-sql-logic-tests Passed (0 seconds)
Details
buildkite/tests/bulb-short-sql-logic-tests Passed (2 minutes, 18 seconds)
Details
buildkite/tests/cargo-test Passed (1 minute, 2 seconds)
Details
buildkite/tests/docker-build Passed (18 minutes, 43 seconds)
Details
buildkite/tests/face-with-monocle-miri-test Passed (3 minutes, 19 seconds)
Details
buildkite/tests/metabase-demo Passed (2 minutes, 3 seconds)
Details
buildkite/tests/paperclip-clippy-and-doctests Passed (6 minutes, 8 seconds)
Details
buildkite/tests/pipeline Passed (1 minute, 7 seconds)
Details
buildkite/tests/racing-car-testdrive Passed (3 minutes, 14 seconds)
Details
buildkite/tests/shower-streaming-demo Passed (1 minute, 31 seconds)
Details
license/cla Contributor License Agreement is signed.
Details
netlify/materializeinc/deploy-preview Deploy preview canceled.
Details
@benesch benesch deleted the benesch:neg-mult branch Mar 15, 2020
@benesch

This comment has been minimized.

Copy link
Member Author

benesch commented Mar 15, 2020

Thanks for the quick review!

@frankmcsherry

This comment has been minimized.

Copy link
Member

frankmcsherry commented Mar 15, 2020

Thank you for the explanation! I learned something. :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.