Snapshot pool integration, first version #673

letFunny · 2024-07-17T09:05:51Z

This PR integrates the pool with the snapshot installation. This is a first version without the real implementation for the callbacks and I/O. It solves a few issues found along the way, namely it includes:

pool integration for background work,
emulating the async nature of rpc sending by scheduling a callback using libuv,
first versions of rpc_fill and is_a_trigger, that means that the code can now generate primitive messages and expect the correct ones,
tests now assert that the messages sent are the correct ones,
added missing state which I noticed when writing new tests.

See the commit list for more information as the functionality is split into self-contained units.

codecov · 2024-07-17T09:16:27Z

Codecov Report

Attention: Patch coverage is 84.23237% with 38 lines in your changes missing coverage. Please review.

Project coverage is 77.87%. Comparing base (054b511) to head (defc3f3).
Report is 12 commits behind head on master.

Files	Patch %	Lines
src/raft/recv_install_snapshot.c	70.00%	8 Missing and 22 partials ⚠️
src/lib/threadpool.c	50.00%	4 Missing and 2 partials ⚠️
test/raft/unit/test_snapshot.c	98.44%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #673      +/-   ##
==========================================
+ Coverage   77.84%   77.87%   +0.02%     
==========================================
  Files         197      197              
  Lines       27761    27953     +192     
  Branches     5493     5529      +36     
==========================================
+ Hits        21611    21767     +156     
- Misses       4333     4346      +13     
- Partials     1817     1840      +23

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

just-now

The first series of comments to test_snapshot.c. review is in progress.

just-now · 2024-07-17T10:46:45Z

src/raft/recv_install_snapshot.c

+		if (sm_state(&leader->rpc.sm) != RPC_FILLED) {
+			return false;
+		}
+		return state == LS_PAGE_READ


maybe it's worth defining a macro/function IN() with the following semantics: IN(state, (LS_PAGE_READ, ..., LS_CHECK_F_HAS_SIGS))

I have changed the implementation locally to use the IN macro and the only problem is that the macro might be difficult to read and debug, thoughts?

#define IN_1(E, X) E == X #define IN_2(E, X, ...) E == X || IN_1(E,__VA_ARGS__) #define IN_3(E, X, ...) E == X || IN_2(E,__VA_ARGS__) #define IN_4(E, X, ...) E == X || IN_3(E,__VA_ARGS__) #define IN_5(E, X, ...) E == X || IN_4(E,__VA_ARGS__) #define IN_6(E, X, ...) E == X || IN_5(E,__VA_ARGS__) #define IN_7(E, X, ...) E == X || IN_6(E,__VA_ARGS__) #define IN_8(E, X, ...) E == X || IN_7(E,__VA_ARGS__) #define IN_9(E, X, ...) E == X || IN_8(E,__VA_ARGS__) #define GET_IN_MACRO(_1,_2,_3,_4,_5,_6,_7,_8,_9,NAME,...) NAME #define IN(E, ...) \ GET_IN_MACRO(__VA_ARGS__,IN_9,IN_8,IN_7,IN_6,IN_5,IN_4,IN_3,IN_2,IN_1)(E,__VA_ARGS__)

Let's discuss this with @cole-miller

just-now · 2024-07-17T11:24:27Z

test/raft/unit/test_snapshot.c

+/* Decorates the callback used when the pool work is done to set the test
+ * fixture flag to true, then calls the original callback.*/
+static void test_fixture_work_cb(pool_work_t *w) {
+	global_fixture.work_done = true;


Seems a good time to invent an implementation of is_main_thread() macro or a function is much closer than we thought... When I looked an wait_work() it wasn't obvious that the spin-lock runs in the same thread with test_fixture_work_cb().

Please put a // PRE(is_main_thread()) to both functions.

I will also add some comments to the global_fixture and these functions.

test/raft/unit/test_snapshot.c

just-now · 2024-07-17T11:42:17Z

test/raft/unit/test_snapshot.c

+	data->cb(data->s, 0);
+}
+
+int uv_sender_send_op(struct sender *s,


If I'm not mistaken, uv_sender_*() functions can be static.

I always forget static and const, always.

just-now · 2024-07-17T11:46:12Z

test/raft/unit/test_snapshot.c

+		.ht_create = pool_ht_create_op,
+		.work_queue = pool_work_queue_op,
+		.sender_send = uv_sender_send_op,
+		.is_main_thread = pool_is_main_thread_op,


It's better not to override is_main_thread().
You have an implementation of pool_is_main_thread_op()
Maybe, it's worth thinking about an implementation designed around uv_key_create(). We need to check if it's a good idea to mix __tls and uv_

What is __tls? EDIT: probably "thread-local-storage".

just-now · 2024-07-17T11:53:15Z

test/raft/unit/test_snapshot.c

+                   MUNIT_UNUSED void *user_data)
+{
+	/* Prevent hangs. */
+	alarm(2);


This is so if you do some changes and the tests breaks in a way that makes wait_work or wait_msg_sent stall, then the execution is terminated after 2s and the test fails but you can still see the results of the rest.

test/raft/unit/test_snapshot.c

just-now

I like this patch.

test/raft/unit/test_snapshot.c

just-now · 2024-07-17T17:11:30Z

src/raft/recv_install_snapshot.c

+		if (sm_state(&leader->rpc.sm) != RPC_FILLED) {
+			return false;
+		}
+		return state == LS_PAGE_READ


Let's discuss this with @cole-miller

just-now · 2024-07-17T17:12:24Z

src/raft/recv_install_snapshot.c

+	PRE(incoming != M_MSG_SENT && incoming != M_TIMEOUT &&
+			incoming != M_WORK_DONE);


Do we really need it here?
It's quite clear from the lines above that this assumption is obvious.

The only reason is that from this line on this is a valid pointer. If you were to do msg->type on M_MSG_SENT, or any of the others, the program would crash.

just-now · 2024-07-17T17:17:13Z

src/raft/recv_install_snapshot.c

 	if (UNLIKELY(rc != 0)) {
 		sm_move(&rpc->sm, RPC_ERROR);
 		return;
 	}
-
 	follower_tick(follower, M_MSG_SENT);
 }

 static bool is_a_trigger_leader(const struct leader *leader, const struct raft_message *incoming)


Does it seem less noisy? What do you think about such a style?

static bool is_a_trigger_leader(const struct leader *leader, const struct raft_message *incoming) { int state = sm_state(&leader->sm); return ergo(incoming == M_WORK_DONE, IN(state, (LS_HT_WAIT, LS_PAGE_SENT, LS_PERSISTED_SIG_PART, LS_PAGE_READ))) && ergo(incoming == M_MSG_SENT, sm_state(&leader->rpc.sm) != RPC_FILLED && IN(state, (LS_PAGE_READ, LS_SNAP_DONE, LS_F_NEEDS_SNAP, LS_REQ_SIG_LOOP, LS_CHECK_F_HAS_SIGS))) && ergo(incoming == M_TIMEOUT, IN(state, (LS_PAGE_READ, LS_SNAP_DONE, LS_F_NEEDS_SNAP, LS_REQ_SIG_LOOP, LS_CHECK_F_HAS_SIGS, LS_WAIT_SIGS))) && ergo(state == LS_F_ONLINE, incoming->type == RAFT_IO_APPEND_ENTRIES_RESULT) && ergo(sm_state(&leader->rpc.sm) != RPC_SENT, false) && ergo(state == LS_PAGE_READ, IN(incoming->type, (RAFT_IO_INSTALL_SNAPSHOT_CP_RESULT RAFT_IO_INSTALL_SNAPSHOT_MV_RESULT))) && ergo(IN(state, (LS_SNAP_DONE, LS_F_NEEDS_SNAP)), incoming->type == RAFT_IO_INSTALL_SNAPSHOT_RESULT) && ergo(IN(state, (LS_REQ_SIG_LOOP, LS_CHECK_F_HAS_SIGS)), incoming->type == RAFT_IO_SIGNATURE_RESULT); }

cc @cole-miller

If we are going for this style I would prefer to have two distinct blocks because in the first three ergos the pointer is not valid M_WORK_DONE, M_MSG_SENT and M_TIMEOUT are equal to 1, 2 and 3, which means that doing incoming->type will crash the program.

My fear is that when we come back and do changes in the future we miss that bit about the ordering because it is not explicit.

cole-miller

A few comments after reading through. This PR is really well-organized, nice work!

test/raft/unit/test_snapshot.c

src/raft/recv_install_snapshot.h

src/lib/threadpool.c

cole-miller

Thanks, this is coming along nicely!

letFunny added 2 commits July 17, 2024 11:10

feat(snapshot): integrate pool, first version

cd35341

bugfix(snapshot): add missing state to follower sm

834489e

letFunny force-pushed the snapshot-pool-integration branch from a1ab273 to fd908d3 Compare July 17, 2024 09:10

just-now reviewed Jul 17, 2024

View reviewed changes

cole-miller reviewed Jul 18, 2024

View reviewed changes

cole-miller self-requested a review July 25, 2024 17:52

cole-miller reviewed Jul 25, 2024

View reviewed changes

src/lib/threadpool.c Outdated Show resolved Hide resolved

src/lib/threadpool.c Outdated Show resolved Hide resolved

cole-miller reviewed Jul 25, 2024

View reviewed changes

src/lib/threadpool.c Outdated Show resolved Hide resolved

cole-miller reviewed Jul 30, 2024

View reviewed changes

src/lib/threadpool.c Outdated Show resolved Hide resolved

cole-miller self-requested a review July 30, 2024 14:19

just-now approved these changes Jul 31, 2024

View reviewed changes

cole-miller approved these changes Jul 31, 2024

View reviewed changes

letFunny force-pushed the snapshot-pool-integration branch from 1fc2144 to 2ce00ea Compare July 31, 2024 15:14

letFunny added 2 commits July 31, 2024 17:50

feat(snapshot): rpc fill and rpc trigger, first version

7de953b

feat: pool thread identification

defc3f3

letFunny force-pushed the snapshot-pool-integration branch from 2ce00ea to defc3f3 Compare July 31, 2024 15:51

cole-miller merged commit 05ae53f into canonical:master Jul 31, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snapshot pool integration, first version #673

Snapshot pool integration, first version #673

letFunny commented Jul 17, 2024

codecov bot commented Jul 17, 2024 •

edited

Loading

just-now left a comment

just-now Jul 17, 2024

letFunny Jul 17, 2024

just-now Jul 17, 2024

just-now Jul 17, 2024

letFunny Jul 18, 2024

just-now Jul 17, 2024

letFunny Jul 18, 2024

just-now Jul 17, 2024

letFunny Jul 17, 2024 •

edited

Loading

just-now Jul 17, 2024

letFunny Jul 17, 2024

just-now left a comment

just-now Jul 17, 2024

just-now Jul 17, 2024

letFunny Jul 18, 2024

just-now Jul 17, 2024

letFunny Jul 18, 2024

cole-miller left a comment

cole-miller left a comment

		PRE(incoming != M_MSG_SENT && incoming != M_TIMEOUT &&
		incoming != M_WORK_DONE);

Snapshot pool integration, first version #673

Snapshot pool integration, first version #673

Conversation

letFunny commented Jul 17, 2024

codecov bot commented Jul 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

letFunny Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

cole-miller left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 17, 2024 •

edited

Loading

letFunny Jul 17, 2024 •

edited

Loading