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

changefeedccl: order validator does not assert monotonic resolved messages #108872

Open
jayshrivastava opened this issue Aug 16, 2023 · 2 comments · May be fixed by #108861
Open

changefeedccl: order validator does not assert monotonic resolved messages #108872

jayshrivastava opened this issue Aug 16, 2023 · 2 comments · May be fixed by #108861
Labels
A-cdc Change Data Capture C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. E-easy Easy issue to tackle, requires little or no CockroachDB experience good first issue T-cdc

Comments

@jayshrivastava
Copy link
Contributor

jayshrivastava commented Aug 16, 2023 •
edited by cockroach-jira-scripts
Loading

We should probably assert in the order validator (which is used in roachtests, changefeed nemeses etc) that resolved messages are monotonic. You should never see a regression in the resolved messages.

I started a draft PR #108861. C2C streaming tests fail because you see resolved messages that go back in time. I don't know if this is expected for streaming. Nevertheless, the PR needs a bit more work.

Jira issue: CRDB-30676
@jayshrivastava jayshrivastava added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-cdc Change Data Capture T-cdc labels Aug 16, 2023
@blathers-crl
Copy link

blathers-crl bot commented Aug 16, 2023

cc @cockroachdb/cdc

@jayshrivastava jayshrivastava mentioned this issue Aug 21, 2023
Merged
@jayshrivastava jayshrivastava linked a pull request Aug 28, 2023 that will close this issue
Open
jayshrivastava added a commit to jayshrivastava/cockroach that referenced this issue Sep 14, 2023
@jayshrivastava
streamingccl: fix incorrect usages of cdctest.Validator
0e1a9d7 
The `NoteResolved` API on the `cdctest.Validator` requires that the passed
resolved timestamp is the resolved timetsamp / frontier for the whole set
of spans being tracked by the validator. Previously, tests would submit resolved
timestamps for subsets of spans at a time, which is incorrect. This would
not cause tests to fail because the order validator does not
assert that resolved timetsamps monotonically increase (tracked by cockroachdb#108872).
This change updates these tests to submit resolved timestamps for the
overall set of spans being tracked. After this change, fixing the
ordering validator in (cockroachdb#108872) will not cause these tests to fail.

Release note: None
Epic: None
jayshrivastava added a commit to jayshrivastava/cockroach that referenced this issue Sep 14, 2023
@jayshrivastava
changefeedccl: assert increasing resolved message timestamps in valid…
3366cfb 
…ators

Previously, no validator in the `cdctest` package would assert that resolved
messages increase monotonically. This is an important ordering guarantee that
changefeeds provide.

This change updates the `orderValidator` to assert this.

Release note: None
Epic: None
Fixes: cockroachdb#108872
@rharding6373 rharding6373 added E-easy Easy issue to tackle, requires little or no CockroachDB experience good first issue labels Mar 21, 2024
@Shwepoo
Copy link

Shwepoo commented Oct 22, 2024 •
edited
Loading

@rharding6373 Hi there! I'd like to pick up this issue and contribute towards resolving it. I have experience working with Go (Golang), and I believe I can help address this effectively.

Could you please provide any additional context or guidelines that I should follow? Looking forward to contributing and collaborating on this.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-cdc Change Data Capture C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. E-easy Easy issue to tackle, requires little or no CockroachDB experience good first issue T-cdc
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

changefeedccl: assert increasing resolved message timestamps in validators jayshrivastava/cockroach
3 participants
@rharding6373 @jayshrivastava @Shwepoo