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

cdc delta == keys does not produce cdc$operation nor cdc$ttl #7095

Closed
elcallio opened this issue Aug 24, 2020 · 8 comments
Closed

cdc delta == keys does not produce cdc$operation nor cdc$ttl #7095

elcallio opened this issue Aug 24, 2020 · 8 comments
Assignees
Milestone

Comments

@elcallio
Copy link
Contributor

If cdc::delta is set to "keys" (keys only), the result is postprocessed to remove certain columns or rows (inefficient - with the latest refactor, it should be easy to just not create it in the first place).

However, even cdc$operation and cdc$ttl is removed. That does not seem right.

@haaawk haaawk self-assigned this Aug 24, 2020
@haaawk
Copy link
Contributor

haaawk commented Aug 24, 2020

It seems that @jul-stas was sure this is correct // We can surely remove "cdc$*" columns.
but I think we need them actually

@elcallio
Copy link
Contributor Author

Indeed. Fixing.

@haaawk
Copy link
Contributor

haaawk commented Aug 24, 2020

I have a patch already

@haaawk
Copy link
Contributor

haaawk commented Aug 24, 2020

Sending it now

@haaawk
Copy link
Contributor

haaawk commented Aug 24, 2020

The fix is here #7096

@elcallio
Copy link
Contributor Author

I think a better fix is to not generate the value parts in the first place, and remove the filtering. With @kbr- :s visitor pattern it is quite straight forward. I have it included in a patch set. I'll send a PR tomorrow.

elcallio pushed a commit to elcallio/scylla that referenced this issue Aug 24, 2020
Fixes scylladb#7095

CDC delta=keys did not include cdc$operation or cdc$ttl, which
both seem rather important info even in keys-only mode.

CDC delta=keys or off both relied on post-filtering
to remove generated log row and/or cells. This is inefficient.
Instead, simply check if the data should be created in the
visitors.
elcallio pushed a commit to elcallio/scylla that referenced this issue Aug 24, 2020
Fixes scylladb#7095

CDC delta=keys did not include cdc$operation or cdc$ttl, which
both seem rather important info even in keys-only mode.

CDC delta=keys or off both relied on post-filtering
to remove generated log row and/or cells. This is inefficient.
Instead, simply check if the data should be created in the
visitors.
nyh pushed a commit that referenced this issue Aug 25, 2020
Fixes #7095

Signed-off-by: Piotr Jastrzebski <piotr@scylladb.com>
@slivne slivne added the bug label Aug 25, 2020
@slivne slivne added this to the 4.3 milestone Aug 25, 2020
elcallio pushed a commit to elcallio/scylla that referenced this issue Aug 26, 2020
Refs scylladb#7095

CDC delta!=full both relied on post-filtering
to remove generated log row and/or cells. This is inefficient.
Instead, simply check if the data should be created in the
visitors.
elcallio pushed a commit to elcallio/scylla that referenced this issue Aug 26, 2020
Refs scylladb#7095

CDC delta!=full both relied on post-filtering
to remove generated log row and/or cells. This is inefficient.
Instead, simply check if the data should be created in the
visitors.
elcallio pushed a commit to elcallio/scylla that referenced this issue Aug 26, 2020
Refs scylladb#7095

CDC delta!=full both relied on post-filtering
to remove generated log row and/or cells. This is inefficient.
Instead, simply check if the data should be created in the
visitors.

v2:
* Fixed delta logs rows created (empty) even when delta == off
elcallio pushed a commit to elcallio/scylla that referenced this issue Aug 26, 2020
Refs scylladb#7095

CDC delta!=full both relied on post-filtering
to remove generated log row and/or cells. This is inefficient.
Instead, simply check if the data should be created in the
visitors.

v2:
* Fixed delta logs rows created (empty) even when delta == off
v3:
* Killed delta == off
elcallio pushed a commit to elcallio/scylla that referenced this issue Aug 26, 2020
Refs scylladb#7095

CDC delta!=full both relied on post-filtering
to remove generated log row and/or cells. This is inefficient.
Instead, simply check if the data should be created in the
visitors.

v2:
* Fixed delta logs rows created (empty) even when delta == off
v3:
* Killed delta == off
v4:
* Move checks into (const) member var(s)
elcallio pushed a commit to elcallio/scylla that referenced this issue Aug 31, 2020
Refs scylladb#7095

CDC delta!=full both relied on post-filtering
to remove generated log row and/or cells. This is inefficient.
Instead, simply check if the data should be created in the
visitors.

v2:
* Fixed delta logs rows created (empty) even when delta == off
v3:
* Killed delta == off
v4:
* Move checks into (const) member var(s)
nyh added a commit that referenced this issue Aug 31, 2020
…tion

Merged pull request #7121
By Calle Wilund:

Refs #7095
Fixes #7128

CDC delta!=full both relied on post-filtering
to remove generated log row and/or cells. This is inefficient.
Instead, simply check if the data should be created in the
visitors.

Also removed delta_mode=off mode.

  cdc: Remove post-filterings for keys-only/off cdc delta generation
  cdc: Remove cdc delta_mode::off
@avikivity
Copy link
Member

@nyh please evaluate for backport.

syuu1228 pushed a commit to syuu1228/scylla that referenced this issue Sep 7, 2020
Refs scylladb#7095

CDC delta!=full both relied on post-filtering
to remove generated log row and/or cells. This is inefficient.
Instead, simply check if the data should be created in the
visitors.

v2:
* Fixed delta logs rows created (empty) even when delta == off
v3:
* Killed delta == off
v4:
* Move checks into (const) member var(s)
@nyh
Copy link
Contributor

nyh commented Sep 9, 2020

Not backporting. These are changes to recently added and refactored code.

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

Successfully merging a pull request may close this issue.

6 participants