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

changefeedccl: Rename and remove CDC functions #96295

Merged
merged 1 commit into from
Feb 15, 2023

Conversation

miretskiy
Copy link
Contributor

@miretskiy miretskiy commented Jan 31, 2023

This PR renames and removes CDC specific functions, while maintaining backward compatability.

  • cdc_is_delete() function removed. It is replaced
    with event_op() function which returns a string describing
    the type of event. If the changefeed is running with diff
    option, then this function returns insert, update, or
    delete. If changefeed is running without the diff option,
    we can't tell an update from insert, so this function returns
    upsert or delete.
  • cdc_mvcc_timestamp() function removed. This information can be accessed
    via cockroach standard system column crdb_internal_mvcc_timestamp.
    The same timestamp column is avaiable in the previous row state
    (cdc_prev).crdb_internal_mvcc_timestamp
  • cdc_updated_timestamp() function renamed as event_schema_timestamp()

Fixes #92482
Epic: CRDB-17161

Release note (enterprise change): Deprecate and replace some of the changefeed specific functions made available in preview mode in 22.2
release. While these functions are deprecated, old changefeed
transformations should continue to function properly.
Customers are encouraged to closely monitor their changefeeds
created during upgrade. While effort was made to maintain backward
compatibility, the updated changefeed transformation may produce
slightly different output (different column names, etc).

@miretskiy miretskiy requested a review from a team as a code owner January 31, 2023 19:43
@miretskiy miretskiy requested review from jayshrivastava and removed request for a team January 31, 2023 19:43
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@jayshrivastava jayshrivastava left a comment

Choose a reason for hiding this comment

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

Small comments. Looks good otherwise.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @HonoreDB and @miretskiy)


-- commits line 5 at r1:
Since they were released in preview mode, it should be fine to break them if you want. Up to you.

Code quote:

  This PR renames and removes CDC specific functions,
  while maintaining backward compatability.

-- commits line 5 at r1:
typo: "compatibility"


-- commits line 9 at r1:
typo: corresponding


-- commits line 14 at r1:
typo: available


pkg/ccl/changefeedccl/cdceval/expr_eval.go line 186 at r1 (raw file):

	// Setup context.
	if err := e.setupContextForRow(ctx, updatedRow, prevRow.IsDeleted()); err != nil {

I think there's some subtitles here. For example, isNew will always be true when you don't use with_diff because the prevRow is always uninitialized (at least I think that's the case from reading the code). IMO that's fine, but unexpected. Maybe that's something worth mentioning in a comment or in the event_op function Info field in pkg/ccl/changefeedccl/cdceval/functions.go

@miretskiy
Copy link
Contributor Author

I think there's some subtitles here. For example, isNew will always be true when you don't use with_diff because the prevRow is always uninitialized (at least I think that's the case from reading the code). IMO that's fine, but unexpected.

This is an excellent point, @jayshrivastava
I need to think a bit more on this; I think we should force withDiff if you use event_op (much like we do w/ cdc_prev).
But, as you say, this might be a simple matter of documenting because forcing withDiff is not w/out costs.

@miretskiy
Copy link
Contributor Author

@jayshrivastava -- just pushed an updated version; I changed this function so that when running without diff,
only upsert or delete is returned since we can't tell insert from an update.
Thanks for bringing this up.

@miretskiy
Copy link
Contributor Author

@jayshrivastava let me know if I still have your vote after the most recent changes.

Copy link
Contributor

@jayshrivastava jayshrivastava left a comment

Choose a reason for hiding this comment

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

LGTM. There are still a couple typos.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @HonoreDB and @miretskiy)


-- commits line 5 at r1:

Previously, jayshrivastava (Jayant) wrote…

typo: "compatibility"

!


-- commits line 14 at r1:

Previously, jayshrivastava (Jayant) wrote…

typo: available

!

Copy link
Contributor Author

@miretskiy miretskiy left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @HonoreDB and @jayshrivastava)


-- commits line 5 at r1:

Previously, jayshrivastava (Jayant) wrote…

Since they were released in preview mode, it should be fine to break them if you want. Up to you.

Done.


pkg/ccl/changefeedccl/cdceval/expr_eval.go line 186 at r1 (raw file):

Previously, jayshrivastava (Jayant) wrote…

I think there's some subtitles here. For example, isNew will always be true when you don't use with_diff because the prevRow is always uninitialized (at least I think that's the case from reading the code). IMO that's fine, but unexpected. Maybe that's something worth mentioning in a comment or in the event_op function Info field in pkg/ccl/changefeedccl/cdceval/functions.go

Done.

This PR renames and removes CDC specific functions,
while maintaining backward compatability.

* `cdc_is_delete()` function removed.  It is replaced
  with `event_op()` function which returns a string describing
  the type of event.  If the changefeed is running with `diff`
  option, then this function returns `insert`, `update`, or
  `delete`.  If changefeed is running without the `diff` option,
  we can't tell an update from insert, so this function returns
  `upsert` or `delete`.
* `cdc_mvcc_timestamp()` function removed.  This information
  can be accessed via cockroach standard system column
  `crdb_internal_mvcc_timestamp`.  The same timestamp column
  is available in the previous row state
  `(cdc_prev).crdb_internal_mvcc_timestamp`
* `cdc_updated_timestamp()` function renamed as
  `event_schema_timestamp()`

Fixes cockroachdb#92482
Epic: CRDB-17161

Release note (enterprise change): Deprecate and replace some of the
changefeed specific functions made available in preview mode in 22.2
release.   While these functions are deprecated, old changefeed
transformations should continue to function properly.
Customers are encouraged to closely monitor their changefeeds
created during upgrade.  While effort was made to maintain backward
compatability, the updated changefeed transformation may produce
slightly different output (different column names, etc).
@miretskiy
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 14, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Feb 15, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Feb 15, 2023

Build succeeded:

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

Successfully merging this pull request may close these issues.

Cockroachdb cdc debezium format miss “OP”
3 participants