Fetch response queue #63

jost-s · 2024-12-31T02:09:38Z

Adds a simple response queue to the fetch module which processes fetch requests one by one by loading requested ops from an op store and sending the available ops to the requester.

depends on #60

resolves #30

jost-s · 2025-01-03T02:06:33Z

crates/api/proto/fetch.proto

+// A list of ops.
+message Ops {
+  // Ops.
+  repeated kitsune2.op_store.Op data = 1;


Re-use op definition.

jost-s · 2025-01-03T02:09:26Z

crates/api/src/fetch.rs

+}
+
+/// Serialize list of ops for sending over the wire.
+pub fn serialize_ops(value: Vec<MetaOp>) -> bytes::Bytes {


Very similar to the previous op id one. I could write this as a generic function which saves us code here but requires then types when calling the function.

jost-s · 2025-01-03T02:10:58Z

crates/core/src/factories/core_fetch/back_off.rs

@@ -157,191 +147,4 @@ mod test {

        assert!(!back_off_list.is_agent_on_back_off(&agent_id));
    }
-


These tests are just moved to the request_queue.rs file.

jost-s · 2025-01-03T02:12:11Z

crates/core/src/factories/core_fetch/test.rs

This file contains now shared util functions. Tests for the parts of the fetch module are split out into their own test files.

jost-s · 2025-01-03T02:12:39Z

crates/core/src/factories/core_fetch/test/request_queue.rs

No new test cases here, just updated API.

ThetaSinner

Looking good, I like the approach and most of my comments are about naming and testing.

ThetaSinner · 2025-01-03T15:13:52Z

crates/api/proto/op_store.proto

+// An op.
+message Op {
+    // Op id.
+    bytes op_id = 1;


Want to question the value of sending the op_id with the Op. When we're communicating a diff, we have to work in terms of op_ids but when we have the op_data, we shouldn't trust the op_id that's been provided, we should calculate it from the op_data. This links to what we were discussing on the DHT diff PR where hash length mismatches would cause an error, we need to be properly checking these as they arrive.

That's true. I'll refactor.

Wait, we don't have a hashing for that yet tho. Should I add some temporary hashing for the id?

Yes please. I had to do something similar with receiving ops into the store vs notifying the DHT model that ops have arrived on the host. Maybe it needs to be a proper test utility as part of demonstrating the host. I can't remember exactly where I did this because it's been a couple of weeks, but I think I skipped actually calculating hashes.

I'll have to compute hashes if the ops that are requested by id and then sent without id need to be compared. I can add that as a test utility.

crates/api/src/op_store.rs

ThetaSinner · 2025-01-03T15:25:44Z

crates/core/src/factories/core_fetch.rs

+//! - Simple queue which processes items in the order of the incoming requests.
+//! - Requests consist of a list of requested op ids and the requesting agent id.
+//! - Attempts to look up the op in the data store and send response are done once.
+//! - Requests for data that the remote doesn't hold should be logged.


This looks like it needs a second look? Is this "requests for data we don't have are logged"?

ThetaSinner · 2025-01-03T15:26:11Z

crates/core/src/factories/core_fetch.rs

+//! - Attempts to look up the op in the data store and send response are done once.
+//! - Requests for data that the remote doesn't hold should be logged.
+//! - If none of the requested ops could be read from the store, no response is sent.
+//! - If sending or reception fails, it's the caller's responsibility to request again.


Suggested change

//! - If sending or reception fails, it's the caller's responsibility to request again.

//! - If sending or receiving fails, it's the caller's responsibility to request again.

crates/core/src/factories/core_fetch.rs

crates/core/src/factories/core_fetch/back_off.rs

crates/core/src/factories/core_fetch/test/response_queue.rs

ThetaSinner · 2025-01-03T15:39:33Z

crates/core/src/factories/core_fetch/test/response_queue.rs

+    let requested_ops_1 = vec![op_id_1.clone(), op_id_2.clone()];
+    let requested_ops_2 = vec![op_id_3.clone(), op_id_4.clone()];
+    futures::future::join_all([
+        fetch.respond_with_ops(requested_ops_1, agent_id_1.clone()),


I wonder if request_op_data might be a clearer name for this? I can't quite get this into my head

I don't find the name very comprehensible either. But it's not a request, it's a response to a request which pulls ops from the store and sends them to the requesting agent.

I see what you're saying but in my head it kind of is a request. We're choosing to queue the work in the implementation and this doesn't actually respond in the function return but it is still a remote peer requesting us to do work... Naming is hard :)

Maybe something like handle_op_data_request?

Yes, that's good and fits the handle_incoming_ops in the next PR. I'll change it to handle_op_request. I'd change add_ops to request_ops too.

ThetaSinner · 2025-01-03T15:40:37Z

crates/core/src/factories/core_fetch/test/response_queue.rs

+}
+
+#[tokio::test(flavor = "multi_thread")]
+async fn no_response_sent_when_no_ops_found() {


Possible to also test agent info not in peer store?

and also it'd be great to test error, and then queuing the request again for success. Just to demonstrate that there's no state preventing that working?

- Adds protobuf definition for an individual `MetaOp` to `op_store`. - Adds protobuf definition for a list of ops to `fetch`.

jost-s marked this pull request as ready for review December 31, 2024 02:09

jost-s force-pushed the feat/fetch-response-queue branch from d82ef44 to 4e00657 Compare December 31, 2024 18:27

jost-s commented Jan 3, 2025

View reviewed changes

crates/core/src/factories/core_fetch/test/request_queue.rs Outdated

Copy link

Contributor Author

jost-s Jan 3, 2025

Choose a reason for hiding this comment

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

No new test cases here, just updated API.

jost-s requested a review from a team January 3, 2025 02:13

jost-s mentioned this pull request Jan 3, 2025

Process fetch responses #64

Open

jost-s force-pushed the feat/fetch-response-queue branch from 4e00657 to 57b3547 Compare January 3, 2025 02:32

Base automatically changed from refactor/improve-fetch-back-off-backon to main January 3, 2025 13:07

ThetaSinner reviewed Jan 3, 2025

View reviewed changes

jost-s force-pushed the feat/fetch-response-queue branch from 587bc39 to 0368024 Compare January 3, 2025 16:22

jost-s added 2 commits January 3, 2025 10:26

feat(api): add serialization & deserialization for ops

6b827a1

- Adds protobuf definition for an individual `MetaOp` to `op_store`. - Adds protobuf definition for a list of ops to `fetch`.

feat(proto-build): build definition for op_store

3942008

jost-s force-pushed the feat/fetch-response-queue branch 2 times, most recently from 57d09d2 to edbf162 Compare January 3, 2025 17:22

feat(core): add response queue to fetch module

a841ce5

jost-s force-pushed the feat/fetch-response-queue branch from edbf162 to a841ce5 Compare January 3, 2025 17:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetch response queue #63

Fetch response queue #63

jost-s commented Dec 31, 2024 •

edited

Loading

jost-s Jan 3, 2025

jost-s Jan 3, 2025

jost-s Jan 3, 2025

jost-s Jan 3, 2025

jost-s Jan 3, 2025

ThetaSinner left a comment

ThetaSinner Jan 3, 2025

jost-s Jan 3, 2025

jost-s Jan 3, 2025

ThetaSinner Jan 3, 2025

jost-s Jan 4, 2025

ThetaSinner Jan 3, 2025

ThetaSinner Jan 3, 2025

ThetaSinner Jan 3, 2025

jost-s Jan 3, 2025

ThetaSinner Jan 3, 2025

jost-s Jan 3, 2025

ThetaSinner Jan 3, 2025

ThetaSinner Jan 3, 2025

		@@ -157,191 +147,4 @@ mod test {

		assert!(!back_off_list.is_agent_on_back_off(&agent_id));
		}

	//! - If sending or reception fails, it's the caller's responsibility to request again.
	//! - If sending or receiving fails, it's the caller's responsibility to request again.

Fetch response queue #63

Are you sure you want to change the base?

Fetch response queue #63

Conversation

jost-s commented Dec 31, 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

ThetaSinner 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

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

jost-s commented Dec 31, 2024 •

edited

Loading