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

Update the `since` field of IndexState. #2336

Merged
merged 3 commits into from Mar 16, 2020

Conversation

@frankmcsherry
Copy link
Member

frankmcsherry commented Mar 16, 2020

This field was not previously being updated, which made validity tests against it vacuously true. This meant that races between compaction and upper frontiers could result in incorrect results. There should now be an error instead (the error has always existed, but the state on which it was based was not correct).

This field was not previously being updated, which made validity tests against it vacuously true. This meant that races between compaction and upper frontiers could result in incorrect results. There should now be an error instead (the error has always existed, but the state on which it was based was not correct).
Copy link
Member

benesch left a comment

Whoops!

Copy link
Member

wangandi left a comment

Can't comment on any line outside of a 5 line radius around the changes, but should we remove the #[allow(dead_code)] at line 1888 above the since attribute of IndexState?

@frankmcsherry

This comment has been minimized.

Copy link
Member Author

frankmcsherry commented Mar 16, 2020

That sounds like a great idea! I think it hasn't been dead for a while (it is read, just not written to), but change inbound.

Also some tests are now failing because of this new error. Who knows what they were producing before... ;D

@frankmcsherry

This comment has been minimized.

Copy link
Member Author

frankmcsherry commented Mar 16, 2020

The issues look to be related to the compaction of file sources, which announce themselves as complete at all times. The strategy of just subtracting off a small amount of time doesn't work great here. I think we had a hack to work around that, but it doesn't seem to fix things here. Consulting with @umanwizard.

@frankmcsherry frankmcsherry merged commit bd411fe into MaterializeInc:master Mar 16, 2020
14 checks passed
14 checks passed
buildkite/tests Build #5973 passed (14 minutes, 34 seconds)
Details
buildkite/tests/bath-lint-and-rustfmt Passed (1 minute, 4 seconds)
Details
buildkite/tests/bulb-bulb-full-sql-logic-tests Passed (0 seconds)
Details
buildkite/tests/bulb-short-sql-logic-tests Passed (1 minute, 10 seconds)
Details
buildkite/tests/cargo-test Passed (1 minute, 37 seconds)
Details
buildkite/tests/docker-build Passed (10 minutes, 53 seconds)
Details
buildkite/tests/face-with-monocle-miri-test Passed (56 seconds)
Details
buildkite/tests/metabase-demo Passed (41 seconds)
Details
buildkite/tests/paperclip-clippy-and-doctests Passed (1 minute, 23 seconds)
Details
buildkite/tests/pipeline Passed (15 seconds)
Details
buildkite/tests/racing-car-testdrive Passed (3 minutes, 18 seconds)
Details
buildkite/tests/shower-streaming-demo Passed (27 seconds)
Details
license/cla Contributor License Agreement is signed.
Details
netlify/materializeinc/deploy-preview Deploy preview canceled.
Details
@frankmcsherry

This comment has been minimized.

Copy link
Member Author

frankmcsherry commented Mar 16, 2020

Thanks folks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.