datalake/coordinator: initial coordinator base #23712

andrwng · 2024-10-09T20:41:26Z

Based on #23714

Adds some initial structure for the coordinator. This commit just adds
wrapper around a couple of calls to the underlying STM, so that it can
be plugged into the frontend.

Later commits will have the coordinator reconcile the STM with the
Iceberg catalog, perform local/Raft snapshots, etc.

Backports Required

Release Notes

none

This will be useful for logging.

Pulls some utilities out of a test to make it easier to reuse building state updates.

For use by the upcoming datalake coordinator STM to track files that need to be committed to Iceberg.

Adds topics_state to the STM, and updates the apply method to appropriately update it.

src/v/datalake/coordinator/coordinator.cc

src/v/datalake/coordinator/state_machine.cc

bharathv · 2024-10-10T16:22:58Z

src/v/datalake/coordinator/state_update.cc

    }
    return update;
 }

-bool add_files_update::can_apply(const topics_state& state) {
+checked<std::nullopt_t, stm_update_error>


nit: would be be useful to define a "no error state" in stm_update_error

enum class stm_update_error { none failed, };

that I think removes the need for check<nullopt_t...>

I've been preferring to use errors for only error types, and use result/checked to signal expected vs not expected outcomes. I kind of like how error handling is uniform now with has_error(), instead of having to guess about optionality or special 0 codes

hmm ok, fwiw https://en.cppreference.com/w/cpp/error/error_code/operator_bool also defines using a none equivalent with 0 value, so thats somewhat of a standard approach I guess, anyway I'm not too strong about it.

Yea, IMO C++'s "best practices" for error handling are a bit error prone, unless we standardize our codebase to One True Way, it's way too easy to mess up..

src/v/datalake/coordinator/state_machine.cc

src/v/datalake/coordinator/coordinator.cc

bharathv · 2024-10-10T16:44:10Z

src/v/datalake/coordinator/coordinator.cc

+          "Rejecting request to add files for {}: {}",
+          tp,
+          update_res.error());
+        co_return errc::stm_apply_error;


nit: I think this is a validation error? (since its pre replication).

From the caller's perspective they should be the same thing, no? the request does not apply cleanly to the stm

I was thinking from a debugging perspective, if the request was rejected due to a race or if was legit validation, nbd, feel free to keep it as is.

Leaving it for now, i think the log is enough, unless these get serialized in the response and the only logs we have are the worker logs

In some upcoming changes, we'll expect the STM to be an implementation detail, and for callers to go through a separate coordinator abstraction. To that end, this moves most STM methods out of the public interface and exposes some internals for use in the upcoming coordinator.

Adds some initial structure for the coordinator. This commit just adds wrapper around a couple of calls to the underlying STM, so that it can be plugged into the frontend. Later commits will have the coordinator reconcile the STM with the Iceberg catalog, perform local/Raft snapshots, etc.

vbotbuildovich · 2024-10-10T21:01:35Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/56296#019277d5-c406-497a-9b65-c731729932af
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/56296#019277d5-c40b-47e3-8d9d-484f64cfbebb

vbotbuildovich · 2024-10-10T21:01:58Z

Retry command for Build#56296

please wait until all jobs are finished before running the slash command

/ci-repeat 1
tests/rptest/tests/cloud_storage_timing_stress_test.py::CloudStorageTimingStressTest.test_cloud_storage@{"cleanup_policy":"delete"}
tests/rptest/tests/cloud_storage_timing_stress_test.py::CloudStorageTimingStressTest.test_cloud_storage_with_partition_moves@{"cleanup_policy":"delete"}

dotnwat · 2024-10-10T21:16:21Z

src/v/datalake/coordinator/coordinator.cc

+}
+
+checked<ss::gate::holder, coordinator::errc> coordinator::maybe_gate() {
+    ss::gate::holder h;


h is unused? intentional?

Oops! unintentional, may fix in a follow up if this PR can get merged

github-actions bot added area/build area/redpanda labels Oct 9, 2024

andrwng force-pushed the datalake-coordinator-base branch from eb54b88 to 2115f9d Compare October 9, 2024 22:26

andrwng marked this pull request as ready for review October 9, 2024 22:26

andrwng added 2 commits October 9, 2024 15:34

dl/coordinator: ostream for state_update

531f0ac

dl/coordinator: return message on bad state update

9be8c3e

This will be useful for logging.

andrwng force-pushed the datalake-coordinator-base branch from 2115f9d to 7637549 Compare October 9, 2024 22:37

andrwng added 3 commits October 9, 2024 16:22

dl/coordinator/tests: state_test_utils

4ce066f

Pulls some utilities out of a test to make it easier to reuse building state updates.

model: datalake_coordinator batch type

68c7ea2

For use by the upcoming datalake coordinator STM to track files that need to be committed to Iceberg.

dl/coordinator: apply datalake updates to stm

da7e4e7

Adds topics_state to the STM, and updates the apply method to appropriately update it.

andrwng requested review from mmaslankaprv, bharathv and dotnwat and removed request for mmaslankaprv October 10, 2024 05:40

andrwng mentioned this pull request Oct 10, 2024

datalake/coordinator: add coordinator manager #23720

Merged

7 tasks

mmaslankaprv reviewed Oct 10, 2024

View reviewed changes

src/v/datalake/coordinator/coordinator.cc Show resolved Hide resolved

mmaslankaprv reviewed Oct 10, 2024

View reviewed changes

src/v/datalake/coordinator/state_machine.cc Outdated Show resolved Hide resolved

andrwng force-pushed the datalake-coordinator-base branch from 7637549 to 7ae61d7 Compare October 10, 2024 16:20

andrwng requested a review from mmaslankaprv October 10, 2024 16:24

bharathv reviewed Oct 10, 2024

View reviewed changes

andrwng force-pushed the datalake-coordinator-base branch 2 times, most recently from 46340ac to 89e151f Compare October 10, 2024 17:28

andrwng added 2 commits October 10, 2024 10:29

andrwng force-pushed the datalake-coordinator-base branch from 89e151f to 9ca5fac Compare October 10, 2024 17:29

andrwng requested a review from bharathv October 10, 2024 17:31

andrwng mentioned this pull request Oct 10, 2024

datalake/coordinator: miscellaneous changes #23714

Merged

7 tasks

bharathv approved these changes Oct 10, 2024

View reviewed changes

dotnwat reviewed Oct 10, 2024

View reviewed changes

dotnwat merged commit a599c84 into redpanda-data:dev Oct 10, 2024
14 of 17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

datalake/coordinator: initial coordinator base #23712

datalake/coordinator: initial coordinator base #23712

andrwng commented Oct 9, 2024 •

edited

Loading

bharathv Oct 10, 2024

andrwng Oct 10, 2024

bharathv Oct 10, 2024

andrwng Oct 10, 2024 •

edited

Loading

bharathv Oct 10, 2024

andrwng Oct 10, 2024

bharathv Oct 10, 2024

andrwng Oct 10, 2024

vbotbuildovich commented Oct 10, 2024 •

edited

Loading

vbotbuildovich commented Oct 10, 2024 •

edited

Loading

dotnwat Oct 10, 2024

andrwng Oct 10, 2024

datalake/coordinator: initial coordinator base #23712

datalake/coordinator: initial coordinator base #23712

Conversation

andrwng commented Oct 9, 2024 • edited Loading

Backports Required

Release Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrwng Oct 10, 2024 • edited Loading

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

vbotbuildovich commented Oct 10, 2024 • edited Loading

vbotbuildovich commented Oct 10, 2024 • edited Loading

Retry command for Build#56296

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrwng commented Oct 9, 2024 •

edited

Loading

andrwng Oct 10, 2024 •

edited

Loading

vbotbuildovich commented Oct 10, 2024 •

edited

Loading

vbotbuildovich commented Oct 10, 2024 •

edited

Loading