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

transform-sdk/rust: multiple output topics #17007

Merged
merged 8 commits into from
Mar 26, 2024

Conversation

rockwotj
Copy link
Contributor

Add support for the new method to write options in v2 of the broker's
ABI. This includes specifying write options in the RecordWriter
interface that can then be used to call the new write_with_options
method with the specified topic name.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x

Release Notes

Features

  • The rust transform-sdk gains the ability to write to multiple output topics.
    This feature can only be used in Redpanda v24.1.x or newer.

@rockwotj rockwotj requested a review from a team as a code owner March 11, 2024 20:45
@rockwotj rockwotj requested review from ivotron and removed request for a team March 11, 2024 20:45
@github-actions github-actions bot added area/build area/wasm WASM Data Transforms labels Mar 11, 2024
@rockwotj rockwotj requested a review from a team March 11, 2024 20:53
@rockwotj
Copy link
Contributor Author

rockwotj commented Mar 11, 2024

@redpanda-data/documentation requesting a review for updates to data transforms SDK. The relevant changes are in src/transform-sdk/rust/core-types/src/lib.rs

@rockwotj
Copy link
Contributor Author

Force push: Rebase against dev

impl<'a> WriteOptions<'a> {
/// Create a new options struct with the record destination to an optional `topic`.
///
/// If `topic` is `None` then the record will be written to the first
Copy link
Contributor

@Deflaimun Deflaimun Mar 19, 2024

Choose a reason for hiding this comment

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

Is None a rust syntax? Can the value be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rust doesn't have null - None for an Optional is semantically equivalent.

@@ -116,13 +116,36 @@ impl<'a> From<&'a WrittenRecord<'a>> for BorrowedRecord<'a> {
}
}

/// WriteOptions allows for customizing a RecordWriter's write.
Copy link
Contributor

@Deflaimun Deflaimun Mar 19, 2024

Choose a reason for hiding this comment

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

I'm guessing that WriteOptions and RecordWriter are both functions that the user can use? If yes, should we encompass them in code brackets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes good idea, let me do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better

Screenshot 2024-03-19 at 1 46 09 PM

@Deflaimun
Copy link
Contributor

@rockwotj do you have an example output of those docs? Is this something similar to JavaDoc for example?

Copy link
Member

@ivotron ivotron left a comment

Choose a reason for hiding this comment

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

LGTM on the GH workflow side

@rockwotj rockwotj force-pushed the rust-mot branch 3 times, most recently from 9cd01fe to 8ed4c85 Compare March 19, 2024 18:52
@rockwotj
Copy link
Contributor Author

Force push: docs feedback, then rebase against dev

@Deflaimun yes docs can be generated via cargo doc then there is some static pages at target/doc. I've uploaded an archive of the latest push if you want to take a look.

docs.tar.gz

@rockwotj rockwotj force-pushed the rust-mot branch 2 times, most recently from 8184ce4 to 9ac5608 Compare March 19, 2024 19:03
@rockwotj rockwotj requested a review from Deflaimun March 19, 2024 19:07
Copy link

@kbatuigas kbatuigas left a comment

Choose a reason for hiding this comment

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

Request to fix one apparent typo, questions for clarification, a couple of nitpicks

src/transform-sdk/rust/core-types/src/lib.rs Outdated Show resolved Hide resolved
@@ -116,13 +116,36 @@ impl<'a> From<&'a WrittenRecord<'a>> for BorrowedRecord<'a> {
}
}

/// [`WriteOptions`] allows for customizing a [`RecordWriter`]'s write.
///
/// An example is to customize the output topic which a write can go to.

Choose a reason for hiding this comment

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

Suggested change
/// An example is to customize the output topic which a write can go to.
/// For example, use [`WriteOptions`] to customize the output topic to write to.

Are there any behaviors or cases (other than the output topic) that WriteOptions can be used to customize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not now, but in the future maybe

src/transform-sdk/rust/core-types/src/lib.rs Outdated Show resolved Hide resolved
src/transform-sdk/rust/core-types/src/lib.rs Outdated Show resolved Hide resolved
}

impl<'a> WriteOptions<'a> {
/// Create a new options struct with the record destination to an optional `topic`.

Choose a reason for hiding this comment

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

Is "record" here referring to Record specifically? And would "the record's destination" still be accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Updated PTAL

Self { topic }
}

/// Create a new options struct with the record destination to `topic`.

Choose a reason for hiding this comment

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

Same question as above regarding record/Record

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, updated it, PTAL

src/transform-sdk/rust/core-types/src/lib.rs Outdated Show resolved Hide resolved
src/transform-sdk/rust/core-types/src/lib.rs Outdated Show resolved Hide resolved
src/transform-sdk/rust/core-types/src/lib.rs Outdated Show resolved Hide resolved
@rockwotj
Copy link
Contributor Author

rockwotj commented Mar 23, 2024

Force push respond to documentation feedback

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
@rockwotj
Copy link
Contributor Author

Force push: fix conflicts with dev

@rockwotj
Copy link
Contributor Author

Force push: add some debugging code that was helpful.

NOTE: ccbb347 will be reverted when #17370 is completed

kbatuigas
kbatuigas previously approved these changes Mar 26, 2024
Copy link
Contributor

@michael-redpanda michael-redpanda left a comment

Choose a reason for hiding this comment

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

looks good, just a couple of chores and some questions

src/transform-sdk/rust/core-sys/src/lib.rs Show resolved Hide resolved
src/transform-sdk/rust/core-types/src/lib.rs Show resolved Hide resolved
src/transform-sdk/rust/examples/tee.rs Outdated Show resolved Hide resolved
src/transform-sdk/tests/admin_client.go Show resolved Hide resolved
Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
This is a false positive, as it doesn't understand our test assertions.

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
Currently, some leadership flapping happens when creating this many
topics at once. Instead, we should fix the offset when we deploy, there
is a followup ticket for this and once that is implemented we can revert
this commit.

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
@rockwotj
Copy link
Contributor Author

Force push: remember what year it is

Copy link
Contributor

@michael-redpanda michael-redpanda left a comment

Choose a reason for hiding this comment

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

lgtm

@rockwotj rockwotj merged commit 5bb1f00 into redpanda-data:dev Mar 26, 2024
18 checks passed
@rockwotj rockwotj deleted the rust-mot branch March 26, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build area/wasm WASM Data Transforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants