initial snapshot state machine implementation #659

letFunny · 2024-06-10T11:15:24Z

No description provided.

cole-miller · 2024-06-10T12:02:07Z

Makefile.am

-  src/raft/flags.c \
-  src/raft/heap.c \
-  src/raft/log.c \
+  $(basic_dqlite_sources) \


I'm a bit concerned about making the raft test artifacts link in arbitrary dqlite object code or even libdqlite itself---feels like we should maintain a separation here even if it requires compiling a subset of source files (like tracing.c) more than once.

The problem is that we are going to be using code from dqlite in Raft soon. We need to read pages, check the wal, etc. Right now I needed for the state machines but in the future I thought that we were going to combine both Raft and dqlite.

cole-miller · 2024-06-10T12:03:54Z

Makefile.am

@@ -339,6 +337,9 @@ libsqlite3_la_SOURCES = sqlite3.c
 libsqlite3_la_CFLAGS = -g3

 unit_test_LDADD += libsqlite3.la
+if BUILD_RAFT_ENABLED
+raft_core_unit_test_LDADD += libsqlite3.la


I think tests that use libsqlite3 should be in the dqlite part of the tree.

We are using sqlite to create the hash-table, it does not have anything to do with dqlite per se. The raft code itself (code for creating the ht) has a dependency on SQLite, not only the tests. We could abstract it away to maintain Raft more agnostic but I am not sure there is a point any more in the extra complexity.

cole-miller · 2024-06-10T12:04:39Z

src/raft.h

+typedef unsigned int checksum_t;
+typedef unsigned long int pageno_t;


I would prefer fixed-size integers here, also pageno_t should be only 32 bits (that this is enough is fixed by the SQLite file formats).

cole-miller · 2024-06-10T12:05:07Z

src/raft.h

+
+	const char *db;
+	struct page_from_to page_from_to;
+	unsigned int cs_page_no;


cole-miller · 2024-06-10T12:06:05Z

src/raft/recv_install_snapshot.c

+#include "src/raft.h"
+#include "src/raft/recv_install_snapshot.h"
+#include "src/raft/uv_os.h"


I think the relative paths here are wrong, they should be ../raft.h etc. (Arguably we should switch to just passing -Isrc into the build so we can dispense with all the .. stuff but for now that's where we're at.)

cole-miller · 2024-06-10T12:09:20Z

src/raft/recv_install_snapshot.c

+
+	struct insert_checksum_data *data = (struct insert_checksum_data *)req->data;
+	sqlite3_stmt *stmt = data->state->ht_stmt;
+	for (unsigned int i = 0; i < data->cs_nr; i++) {


unsigned not unsigned int

cole-miller · 2024-06-10T12:09:52Z

src/raft/recv_install_snapshot.c

-	//}
+	int rv;
+
+	struct insert_checksum_data *data = (struct insert_checksum_data *)req->data;


You should be able to drop the explicit cast here.

cole-miller · 2024-06-10T12:11:39Z

src/raft/recv_install_snapshot.c

+
+	struct create_ht_data *data = (struct create_ht_data *)req->data;
+	struct snapshot_leader_state *state = data->state;
+	sprintf(db_filename, "ht-%lld", state->follower_id);


cole-miller · 2024-06-10T12:15:19Z

src/raft.h

+	OK = 0,
+	UNEXPECTED = 1,
+	DONE = 2,


I think these names should be prefixed (RAFT_RESULT_OK etc.)

cole-miller · 2024-06-10T12:19:27Z

src/raft/recv_install_snapshot.c

+	assert(leader != NULL);
+	assert(msg != NULL);


cole-miller · 2024-06-10T12:23:28Z

src/raft/recv_install_snapshot.c

+__attribute__((unused)) static bool leader_invariant(const struct sm *sm,
+						     int prev_state)
+{
+	bool rv;


I would prefer that rv always be an int (that is, to use a different name here).

src/raft/recv_install_snapshot.h

cole-miller · 2024-06-10T12:24:32Z

src/raft/recv_install_snapshot.h

+	unsigned long last_page;
+	unsigned long last_page_acked;


cole-miller · 2024-06-10T12:25:15Z

src/raft/recv_install_snapshot.h

+	sqlite3 *ht;
+	sqlite3_stmt *ht_stmt;


Generally I think we want to have all mentions of sqlite3 types and functions on the dqlite side of the boundary.

src/raft/recv_install_snapshot.h

just-now · 2024-06-10T11:32:18Z

src/raft/recv_install_snapshot.c

+	},
+	[LS_SIGNATURES_CALC_STARTED] = {
+		.flags = 0,
+		.name = "signatures_calc_started",


Note (action is not needed): .name is a label which will be plotted on observability diagrams as a state name.

just-now · 2024-06-10T11:35:45Z

src/raft.h

+typedef unsigned int checksum_t;
+typedef unsigned long int pageno_t;
+
+struct page_checksum_t {


We don't have this as a well-defined style (I believe it is a general practice), still, typically _t postfixes are being used for types defined with typedefs.

just-now · 2024-06-10T13:01:47Z

src/raft/recv_install_snapshot.c

+void send_snapshot_cb(struct raft_io_send *req, int status) {
+	(void)req;
+	(void)status;
+	struct snapshot_leader_state *state = (struct snapshot_leader_state *) req->data;


not sure if it is a good moment for this (but it's always not late to step on the path of samurai).

It looks more maintainable and better from perspective of code analysis (there's no guarantee that req->data lives the same live as the structure containing it) just to embed struct raft_io_send into struct snapshot_leader_state and use container_of() to obtain struct snapshot_leader_state back in the function.

Wouldn't that be a problem in the context of scheduling async callbacks? We could have several raft_io_send when processing several messages. Or should we control that and not process more than 1 message?

So far we have one, still no one prevents us from having a flexible array of raft_io_send structures if needed.

struct x { unsigned sends_nr; struct raft_io_send sends[]; }

just-now · 2024-06-10T13:38:37Z

src/raft/recv_install_snapshot.c

+	PRE(msg->server_id == state->follower_id);
+	// TODO: timeouts.
+	int leader_state = sm_state(leader);
+	if (leader_state == LS_FOLLOWER_ONLINE) {


Suggestion: Case construction looks more idiomatic and natural here.

I was using a switch statement and had some problems when omitting one break; and also when creating variables as case does not create a scope.

Not pushing for switch/case; JFYI: the following syntax is available:

switch (x) { case Y: { } }

just-now · 2024-06-10T13:43:14Z

src/raft/recv_install_snapshot.c

+	struct snapshot_leader_state *state =
+		CONTAINER_OF(sm, struct snapshot_leader_state, sm);
+
+	if (sm_state(sm) == LS_SIGNATURES_CALC_STARTED ||


if it's more useful, you can use >= for states here.

src/raft/recv_install_snapshot.c

just-now · 2024-06-11T12:28:13Z

src/raft/recv_install_snapshot.c

+void send_snapshot_cb(struct raft_io_send *req, int status) {
+	(void)req;
+	(void)status;
+	struct snapshot_leader_state *state = (struct snapshot_leader_state *) req->data;


So far we have one, still no one prevents us from having a flexible array of raft_io_send structures if needed.

struct x { unsigned sends_nr; struct raft_io_send sends[]; }

just-now · 2024-06-11T12:29:52Z

src/raft/recv_install_snapshot.c

+	PRE(msg->server_id == state->follower_id);
+	// TODO: timeouts.
+	int leader_state = sm_state(leader);
+	if (leader_state == LS_FOLLOWER_ONLINE) {


Not pushing for switch/case; JFYI: the following syntax is available:

switch (x) { case Y: { } }

codecov · 2024-06-24T08:41:47Z

Codecov Report

Attention: Patch coverage is 87.53799% with 41 lines in your changes missing coverage. Please review.

Project coverage is 70.91%. Comparing base (6633bd8) to head (ee07596).
Report is 34 commits behind head on master.

Files	Patch %	Lines
src/raft/recv_install_snapshot.c	82.75%	29 Missing and 11 partials ⚠️
test/raft/unit/test_snapshot.c	98.96%	1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (6633bd8) and HEAD (ee07596). Click for more details.

HEAD has 4 uploads less than BASE

Flag BASE (6633bd8) HEAD (ee07596)

6 2

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #659      +/-   ##
==========================================
- Coverage   77.41%   70.91%   -6.50%     
==========================================
  Files         196      196              
  Lines       27269    26664     -605     
  Branches     5455     2833    -2622     
==========================================
- Hits        21111    18910    -2201     
- Misses       4297     5419    +1122     
- Partials     1861     2335     +474

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

just-now

Good work!

src/raft/recv_install_snapshot.c

just-now · 2024-07-01T06:57:13Z

src/raft/recv_install_snapshot.h

+	sender_cb_op cb;
+};
+
+struct timeout {


We had a talk about it, still I'll leave this comment here.

It could be beneficial to embed real implementation of timeout into timeout structure itself. Implementation would be ignored by the test code and will be used by the real implementation or in integration tests.

Same for work- and sender- structs

Agree, I was planning to introduce that gradually in the next patches.

just-now · 2024-07-01T06:59:54Z

src/raft/recv_install_snapshot.h

+enum leader_states {
+	LS_F_ONLINE,
+	LS_HT_WAIT,
+	LS_F_NEEDS_SNAP,
+	LS_CHECK_F_HAS_SIGS,
+	LS_WAIT_SIGS,
+
+	LS_REQ_SIG_LOOP,
+	LS_RECV_SIG_PART,
+	LS_PERSISTED_SIG_PART,
+
+	LS_READ_PAGES_LOOP,
+	LS_PAGE_READ,
+	LS_PAGE_SENT,
+
+	LS_SNAP_DONE,
+	LS_FINAL,
+
+	LS_NR,
+};


These names were possessed from other patches and can be updated according to your naming style (for instance, follower state names names are good), still please don't make them too long.

just-now · 2024-07-01T07:03:09Z

src/raft/recv_install_snapshot.h

+	FS_SIGS_CALC_MSG_RECEIVED,
+	FS_SIGS_CALC_DONE,
+
+	FS_SIG_RECEIVING,


Nit: The leader's SMs denote such states as XYZ_LOOP. I'm fine with XYZ_*ING either, however, please generalize this semantics in the next patches (don't do this right away, still have it in mind).

In the beginning the names were uniform but I wanted to convey somehow that the leader has a loop that involves sending messages and waiting for replies or timeouts, but the follower waits indefinitely for messages, the loop so to speak has different semantics.

I agree that the names should be more uniform in next patches.

cole-miller

Thanks, this is great!

First version of state machines for leader and follower. Included very rudimentary unit tests which show progress as an event log and which can be easily converted to a graph.

cole-miller self-requested a review June 10, 2024 11:57

cole-miller reviewed Jun 10, 2024

View reviewed changes

just-now reviewed Jun 10, 2024

View reviewed changes

just-now reviewed Jun 11, 2024

View reviewed changes

letFunny force-pushed the initial-snapshot-state-machine branch 2 times, most recently from 04087ac to 1619f9d Compare June 12, 2024 14:24

letFunny changed the title ~~[WIP] initial snapshot state machine implementation~~ initial snapshot state machine implementation Jun 27, 2024

letFunny marked this pull request as ready for review June 27, 2024 10:20

just-now approved these changes Jul 1, 2024

View reviewed changes

cole-miller self-requested a review July 2, 2024 14:58

cole-miller approved these changes Jul 2, 2024

View reviewed changes

feat: first version of snapshot state machine

ee07596

First version of state machines for leader and follower. Included very rudimentary unit tests which show progress as an event log and which can be easily converted to a graph.

letFunny force-pushed the initial-snapshot-state-machine branch from abe3384 to ee07596 Compare July 2, 2024 15:02

cole-miller merged commit 921c368 into canonical:master Jul 2, 2024
17 of 18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial snapshot state machine implementation #659

initial snapshot state machine implementation #659

letFunny commented Jun 10, 2024

cole-miller Jun 10, 2024

letFunny Jun 10, 2024

cole-miller Jun 10, 2024

letFunny Jun 10, 2024

cole-miller Jun 10, 2024

cole-miller Jun 10, 2024

cole-miller Jun 10, 2024

cole-miller Jun 10, 2024

cole-miller Jun 10, 2024

cole-miller Jun 10, 2024

cole-miller Jun 10, 2024

cole-miller Jun 10, 2024

cole-miller Jun 10, 2024

cole-miller Jun 10, 2024

cole-miller Jun 10, 2024

just-now Jun 10, 2024

just-now Jun 10, 2024

just-now Jun 10, 2024

letFunny Jun 10, 2024

just-now Jun 11, 2024

just-now Jun 10, 2024

letFunny Jun 10, 2024

just-now Jun 11, 2024

just-now Jun 10, 2024

just-now Jun 11, 2024

just-now Jun 11, 2024

codecov bot commented Jun 24, 2024 •

edited

Loading

just-now left a comment

just-now Jul 1, 2024

letFunny Jul 1, 2024

just-now Jul 1, 2024

just-now Jul 1, 2024

letFunny Jul 1, 2024

cole-miller left a comment

		typedef unsigned int checksum_t;
		typedef unsigned long int pageno_t;

initial snapshot state machine implementation #659

initial snapshot state machine implementation #659

Conversation

letFunny commented Jun 10, 2024

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

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

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

codecov bot commented Jun 24, 2024 • edited Loading

Codecov Report

just-now 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

cole-miller left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 24, 2024 •

edited

Loading