transform: list committed offsets #16185

rockwotj · 2024-01-19T17:51:27Z

We need to cleanup committed offsets when transforms are deleted. At the moment we do not do that. In an effort to do that and ensure we have tests for this, I've broken up that change into two parts, this one, which exposes the ability to view committed offsets, and another to do the cleanup. We'll use the infrastructure from this PR to enable the tests for ensuring that the offsets are cleaned up.

Backports Required

Release Notes

Features

Add an internal debugging endpoint to see the committed progress of data transforms.

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

Expose methods in our service to list what offsets are committed. Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

vbotbuildovich · 2024-01-19T23:36:05Z

new failures in https://buildkite.com/redpanda/redpanda/builds/43995#018d23ca-f357-4ef3-8ecd-f2d28a44a4a5:

"rptest.tests.cloud_storage_timing_stress_test.CloudStorageTimingStressTest.test_cloud_storage.cleanup_policy=delete"

new failures in https://buildkite.com/redpanda/redpanda/builds/43995#018d2438-0858-40b3-87d9-e00451053e99:

"rptest.tests.cloud_storage_timing_stress_test.CloudStorageTimingStressTest.test_cloud_storage.cleanup_policy=delete"

oleiman

lgtm. few questions, but nothing blocking I think

oleiman · 2024-01-22T17:58:16Z

src/v/cluster/distributed_kv_stm.h

+        for (const auto& entry : _kvs) {
+            copy.insert(entry);
+            co_await ss::coroutine::maybe_yield();
+        }


Is the bare loop primarily so you can yield after each insert? Is the yield precautionary, or have you seen stalls w/o it?

Is the yield precautionary,

This one :)

oleiman · 2024-01-22T19:22:15Z

src/v/transform/rpc/client.cc

+      ret_t(model::transform_offsets_map{}),
+      [](ret_t agg, ret_t one) {
+          if (agg.has_error()) {
+              return agg;
+          }
+          if (one.has_error()) {
+              return one;
+          }
+          agg.value().merge(one.value());
+          return agg;
+      });


tangential: I've been thinking about this pattern lately. I wonder whether there will be an abstraction for this type of computation included with std::expected at some point.

Yeah it would be nice, although there are a few different policies I could see here.

some of the low level bits are pretty composable, but yeah, I think they'll eventually run into a wall trying to shoehorn Haskell into C++ :P

oleiman · 2024-01-22T19:34:37Z

src/v/transform/rpc/client.h

+    /**
+     * Find all the partitions in the cluster for offset tracking and list all
+     * the transforms they have tracked.
+     */


Micro-nit: I don't really understand this comment. Doesn't this method just list the committed offset for each <transform-id, input partition-id> pair?

Yes, this description is a bit mechanical. Will try to clarify.

oleiman · 2024-01-22T19:47:14Z

src/v/redpanda/admin/transform.cc

-    ss::json::json_list<ss::httpd::transform_json::transform_metadata>
-      list_result;

-    co_return ss::json::json_return_type(
-      ss::json::stream_range_as_array(report.transforms, [](const auto& entry) {
+    co_return ss::json::json_return_type(ss::json::stream_range_as_array(
+      admin::lw_shared_container(std::move(report.transforms)),
+      [](const auto& entry) {


is this related to listing committed offsets? just a bugfix? if the latter, no issue with including as part of this PR, but it might be a good idea to split into a separate commit.

Yes it avoids a copy because json::stream_range_as_array returns std::function not ss::non_copyable_function. I will split into another commit.

Add the ability to list committed offsets we will use this to expose a debug admin endpoint to list the committed offsets of all transforms. Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

This can be useful for debugging transforms. We'll also use it for some internal tests. Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

This will be used by tests to ensure we're doing cleanup correctly. Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

Add a test that we cleanup offsets after deleting a transform. We currently do not do this, and the test assertion that is commentted out would fail. Followup work will be done to uncomment the assertion. Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

`stream_range_as_array` takes a value, so we need to move it to prevent copies every time a list operation happens. Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

rockwotj · 2024-01-22T20:29:06Z

Force push:

Add separate commit for preventing copies during list (we also don't need the pointer there).
Fix comment for rpc method.

oleiman

lgtm

vbotbuildovich · 2024-01-22T22:54:20Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44110#018d3319-8042-423a-87d8-1feb3451ac22

dotnwat

I've broken up that change into two parts, this one, which exposes the ability to view committed offsets, and another to do the cleanup. We'll use the infrastructure from this PR to enable the tests for ensuring that the offsets are cleaned up.

i really appreciate this approach of breaking things up into immediately usable and testable prs.

dotnwat · 2024-02-08T03:26:44Z

src/v/transform/rpc/client.cc

+      boost::irange(0, cfg->partition_count),
+      [this](int32_t id) {
+          auto partition = model::partition_id(id);


it's really unsatisfying to my brain that we are generating the partition ids here rather than having the set of partitions available to iterate over. is that information not available here, or is this code fundamentally acting like a client?

We only know how many partitions there are and we need to map reduce over all of them. This code is a client fwiw. Is there a way to get a materialized set of all partitions?

not sure. but it representing a client fundamentally makes much more sense to me!

github-actions bot added the area/redpanda label Jan 19, 2024

rockwotj added 3 commits January 19, 2024 14:31

cluster/kv_stm: add ability to list offsets

0d26338

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

mode/transform: add alias for transform offsets map

af556e9

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

transform/rpc: allow listing committed offsets

e06476f

Expose methods in our service to list what offsets are committed. Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

rockwotj force-pushed the offset-tracking-cleanup branch 2 times, most recently from 0131062 to f95a1a1 Compare January 19, 2024 20:41

rockwotj marked this pull request as ready for review January 19, 2024 20:41

rockwotj requested review from dotnwat, oleiman and michael-redpanda January 19, 2024 20:41

oleiman reviewed Jan 22, 2024

View reviewed changes

rockwotj added 6 commits January 22, 2024 14:23

transform/rpc/client: add list_committed_offsets

e14c954

Add the ability to list committed offsets we will use this to expose a debug admin endpoint to list the committed offsets of all transforms. Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

admin/transform: support listing committed offsets

e6e4641

This can be useful for debugging transforms. We'll also use it for some internal tests. Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

admin/transform: allow viewing unknown transforms with committed offsets

4dbcda9

This will be used by tests to ensure we're doing cleanup correctly. Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

rptest/admin: support list committed wasm offsets

a83255c

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

admin/transform: prevent copies

e49d787

`stream_range_as_array` takes a value, so we need to move it to prevent copies every time a list operation happens. Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

rockwotj force-pushed the offset-tracking-cleanup branch from f95a1a1 to e49d787 Compare January 22, 2024 20:28

rockwotj requested a review from oleiman January 22, 2024 20:29

oleiman approved these changes Jan 22, 2024

View reviewed changes

rockwotj merged commit 944df5a into redpanda-data:dev Jan 23, 2024
18 checks passed

rockwotj deleted the offset-tracking-cleanup branch January 23, 2024 02:51

rockwotj mentioned this pull request Feb 6, 2024

[v23.3.x] transform: validate wasm binaries at deploy time #16498

Merged

dotnwat reviewed Feb 8, 2024

View reviewed changes

rockwotj mentioned this pull request Feb 8, 2024

transform: gc committed offsets #16526

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transform: list committed offsets #16185

transform: list committed offsets #16185

rockwotj commented Jan 19, 2024

vbotbuildovich commented Jan 19, 2024 •

edited

oleiman left a comment

oleiman Jan 22, 2024

rockwotj Jan 22, 2024

oleiman Jan 22, 2024

rockwotj Jan 22, 2024

oleiman Jan 22, 2024

oleiman Jan 22, 2024

rockwotj Jan 22, 2024

oleiman Jan 22, 2024

rockwotj Jan 22, 2024

rockwotj commented Jan 22, 2024

oleiman left a comment

vbotbuildovich commented Jan 22, 2024

dotnwat left a comment

dotnwat Feb 8, 2024

rockwotj Feb 8, 2024

dotnwat Feb 8, 2024

transform: list committed offsets #16185

transform: list committed offsets #16185

Conversation

rockwotj commented Jan 19, 2024

Backports Required

Release Notes

Features

vbotbuildovich commented Jan 19, 2024 • edited

oleiman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockwotj commented Jan 22, 2024

oleiman left a comment

Choose a reason for hiding this comment

vbotbuildovich commented Jan 22, 2024

dotnwat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbotbuildovich commented Jan 19, 2024 •

edited