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: experimental C++ support #16229

Merged
merged 6 commits into from
Mar 15, 2024

Conversation

rockwotj
Copy link
Contributor

@rockwotj rockwotj commented Jan 22, 2024

Add experimental C++ SDK. It's currently not going to be more formally supported in docs or RPK at the moment.

Most of the implementation here falls out from mirroring similar patterns from the Rust SDK.

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
  • v23.1.x

Release Notes

  • none

@github-actions github-actions bot added area/build area/wasm WASM Data Transforms labels Jan 22, 2024
@rockwotj rockwotj self-assigned this Jan 22, 2024
@rockwotj rockwotj marked this pull request as ready for review January 22, 2024 21:15
@rockwotj rockwotj requested a review from a team as a code owner January 22, 2024 21:15
@rockwotj rockwotj requested review from andrewhsu, BenPope, oleiman, graphcareful and michael-redpanda and removed request for a team January 22, 2024 21:15
@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Jan 22, 2024

andrewhsu
andrewhsu previously approved these changes Jan 23, 2024
Copy link
Member

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

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

LGTM w.r.t. github workflow

@rockwotj
Copy link
Contributor Author

/ci-repeat

@rockwotj rockwotj requested a review from oleiman March 11, 2024 15:34
@rockwotj rockwotj force-pushed the cpp-transform-sdk branch 3 times, most recently from 8ea48cb to 8e12c31 Compare March 11, 2024 18:49
Copy link
Member

@oleiman oleiman 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. As requested focused mostly on the headers and docs. If you're not opposed to squashing up some of the implementation, happy to go through the .cc in more detail as well.

# Experimental Redpanda Data Transforms C++ SDK

This directory contains an experimental C++ SDK for Redpanda Data Transforms.
C++23 is required for both language features and standard library support.
Copy link
Member

Choose a reason for hiding this comment

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

😈

Comment on lines 6 to 7
When writing C++ in WebAssembly, note that certainly language features are not supported.
Noteably C++ exceptions, `setjmp`/`longjmp`, threads and networking is not supported.
Copy link
Member

Choose a reason for hiding this comment

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

q: are some of these fundamental limitations of wasm as a c++ target, or just more things we don't currently support in our wasi implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is more of a wasm32 VM feature we're missing. There is no VM support for non local control flow. It's likely going to happen this year, until then we're stuck with -fno-exceptions (honestly I kind of like it tho).

Copy link
Member

Choose a reason for hiding this comment

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

Cool. I was wondering whether it's worth segmenting into like

  • things we don't support
  • things the VM doesn't support
  • things that can't or will never exist

But given that we're not offering an exhaustive list, maybe not worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically all of these are things are not supported by the VM at the moment (except networking is a wasi limitation). We hope to lift them all in the future (except probably threads). I will add some color here.

src/transform-sdk/cpp/transform_sdk.h Outdated Show resolved Hide resolved
src/transform-sdk/cpp/transform_sdk.cc Outdated Show resolved Hide resolved
src/transform-sdk/cpp/README.md Outdated Show resolved Hide resolved
src/transform-sdk/cpp/transform_sdk.h Outdated Show resolved Hide resolved
src/transform-sdk/cpp/transform_sdk.h Outdated Show resolved Hide resolved
src/transform-sdk/cpp/transform_sdk.h Outdated Show resolved Hide resolved
src/transform-sdk/cpp/src/transform_sdk.cc Outdated Show resolved Hide resolved
src/transform-sdk/cpp/src/transform_sdk.cc Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

std::string generally signifies the data is a valid UTF-8 data,

not sure i understand this part. afaict the standard says std::string stores 1-byte chars it seems like a fine container for bytes. either way tho, uint8_t is better and std::byte also exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sorry, this std::string should be string (UTF-8) data is sort of an assumption that I have picked up from my time at Google, but I do think std::vector<uint8_t> communicates the semantics better. I looked at std::byte and it doesn't plug into existing libraries as well, so I picked something that should integrate a little better with other C/C++ code.

Comment on lines +19 to +22
redpanda::on_record_written(
[](redpanda::write_event event, redpanda::record_writer* writer) {
return writer->write(event.record);
});
Copy link
Member

Choose a reason for hiding this comment

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

well... that's neat

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Glad you like it :)

Hopefully this is a rather easy interface for folks to use. It's very similar to what Rust/Go looks like.

@rockwotj
Copy link
Contributor Author

Force push: Address comments and cleanup the history

Add an experimental C++ SDK. This SDK closely follows the Rust
implementation in terms of API.

We intentionally keep the source to be a single implementation and
header file for easy drop into existing or other build systems.

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
Add a simple CMake build for the C++ SDK.

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
Closely mirrors the config in the root of the Redpanda repo.

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
Add an example of using the C++ SDK for the identity transform. This
inclues a CMake build to integrate the SDK.

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

Force push rebase against dev

Add a README telling folks how to use the SDK along with a short
overview.

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

If you're not opposed to squashing up some of the implementation, happy to go through the .cc in more detail as well.

I squashed it up, still up to you if you want to go through the .cc more! Thanks again for the review

@oleiman oleiman self-requested a review March 14, 2024 15:05
Copy link
Member

@oleiman oleiman left a comment

Choose a reason for hiding this comment

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

Took a non-exhaustive look at transform_sdk.cc, and it looks pretty great. I don't want to hold this up any longer.

src/transform-sdk/cpp/src/transform_sdk.cc Show resolved Hide resolved
src/transform-sdk/cpp/src/transform_sdk.cc Show resolved Hide resolved
@rockwotj rockwotj merged commit d0ec79a into redpanda-data:dev Mar 15, 2024
18 checks passed
@rockwotj rockwotj deleted the cpp-transform-sdk branch March 15, 2024 18:15
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