Query history from history server #143

progval · 2024-10-26T15:01:00Z

sable_history/src/pg_history_service.rs

sable_network/src/history/local_service.rs

sable_network/src/history/service.rs

sable_ircd/src/command/handlers/chathistory.rs

progval · 2024-11-02T11:12:49Z

@spb AFAICT, I only need to implement AROUND in the postgresql backend before this PR is ready to be merged. Could you start reviewing?

spb · 2024-11-02T12:00:35Z

sable_history/src/pg_history_service.rs

+
+        let mut connection_lock = self.database_connection.lock().await;
+
+        let db_channel_id = i64::try_from(channel_id.as_u64()).expect("channel id overflows u64");


I've been preferring as conversions for this (to be replaced eventually with cast_signed/cast_unsigned), to just store and retrieve the bit pattern on the basis that we don't need the database server to be ordering IDs, just checking equality.

Unless we do need the database to order them, in which case we need to change the existing code (and should probably enforce on creation that an id fits in 63 bits). Either way I think it's better to be consistent.

ok for now, but in the future we should do it explicitly, eg. with bytemuck. Behavior of as depends on compilation profile, so it shouldn't be relied upon.

spb · 2024-11-02T12:07:44Z

sable_history/src/pg_history_service.rs

+                return HashMap::new();
+            }
+            Ok(rows) => {
+                rows.map(|row| row.expect("Could not deserialize row"))


Is panicking appropriate here or should we log an error and skip instead? We've got a lot of existing PoC code that panics all over the place but maybe it's time to make more efforts not to.

Based on https://docs.rs/diesel/latest/diesel/result/enum.Error.html this happens when:

there is a major logic error on our side,

the database misbehaved, or

the database schema does not match Rust's

It seems reasonable to crash either way.

Heh, you're right. I removed the panics.

spb · 2024-11-02T12:18:37Z

sable_history/src/pg_history_service.rs

+                historic_users::dsl::ident,
+                historic_users::dsl::vhost,
+                historic_users::dsl::account_name,
+            ))


Is it worth packaging this query up into a helper function in the models?

spb · 2024-11-02T12:22:32Z

sable_history/src/server/mod.rs

@@ -27,7 +29,7 @@ pub struct HistoryServerConfig {
 pub struct HistoryServer {
    node: Arc<NetworkNode>,
    history_receiver: Mutex<UnboundedReceiver<sable_network::rpc::NetworkHistoryUpdate>>,
-    database_connection: Mutex<AsyncPgConnection>,
+    database_connection: Mutex<AsyncPgConnection>, // TODO: use a connection pool


Yes, definitely

sable_network/Cargo.toml

spb · 2024-11-02T12:27:59Z

sable_network/src/history/local_service.rs

@@ -20,12 +22,12 @@ fn target_id_for_entry(for_user: UserId, entry: &HistoryLogEntry) -> Option<Targ
 }

 /// Implementation of [`HistoryService`] backed by [`NetworkNode`]
-pub struct LocalHistoryService<'a> {
-    node: &'a NetworkNode,
+pub struct LocalHistoryService<'a, NetworkPolicy: policy::PolicyService> {


I'm starting to really question whether making Node generic over its policy was a good idea. Not much to do about it in this PR but it wants thought more generally.

sable_network/src/history/mod.rs

spb · 2024-11-09T13:39:49Z

sable_history/src/pg_history_service.rs

+            ))
+            .filter(messages::dsl::target_channel.eq(db_channel_id));
+        Ok(match request {
+            HistoryRequest::Latest { to_ts, limit } => {


Is there no commonality in these arms that can be broken out? They're quite hard to follow as they are.

I can rewrite Latest/Before/After as a Between, I guess

I managed to deduplicate most of the code: a8f711b

spb · 2024-11-09T13:42:15Z

sable_history/src/pg_history_service.rs

+
+fn make_historical_event(
+    channel: &crate::models::Channel,
+    (id, timestamp, message_type, text, source_nick, source_ident, source_vhost, source_account): (


This might be more readable as a new Queryable struct (possibly with a helper method in the model to query it) - this ties into the comment further up about a helper query method

I don't see what you have in mind

spb · 2024-11-09T13:46:38Z

sable_history/src/server/update_handler.rs

+    async fn handle_new_message(
+        &self,
+        new_message: update::NewMessage,
+        update_timestamp: i64,


Why is this not using the ts field from the Message?

To make the @time tag consistent between echo-message and chathistory d56db83

spb · 2024-11-09T13:55:20Z

sable_network/src/network/event/details.rs

        pub sasl_mechanisms: Vec<String>,
    }

+    #[target_type(ServerId)]
+    struct IntroduceHistoryServer {


I realise this is matching the services behaviour and it's probably fine for now, but long term the desired behaviour isn't the same - we need a single authoritative services node, but for history we can have multiple active nodes and just send requests to any of them.

progval added 2 commits October 26, 2024 17:43

[WIP] Query history from history server

298a257

LATEST impl with blanks to fill

b7369a8

progval force-pushed the remote-history-service branch from 8c1f8ed to b7369a8 Compare October 26, 2024 16:37

progval commented Oct 26, 2024

View reviewed changes

sable_history/src/pg_history_service.rs Outdated Show resolved Hide resolved

progval added 3 commits October 27, 2024 08:59

[WIP] decouple send_item

2c0eaad

Make HistoryService return an inflated event instead of HistoryLogEntry

2ef9b6a

Merge branch 'master' into remote-history-service

c93e5bd

progval commented Oct 27, 2024

View reviewed changes

sable_network/src/history/local_service.rs Outdated Show resolved Hide resolved

spb reviewed Oct 27, 2024

View reviewed changes

sable_network/src/history/service.rs Outdated Show resolved Hide resolved

spb reviewed Oct 27, 2024

View reviewed changes

sable_network/src/history/service.rs Outdated Show resolved Hide resolved

progval and others added 7 commits October 27, 2024 15:59

Finish HistoryRequest::Latest in pg_history_service

e5aa0e0

Partial fill-in-the-blanks for message translation

682bbf7

Add missing files

a8874b6

Make target a String in case it expired

f0ac38d

Make id a MessageId

9b88fca

sable_history: Add support for running migrations on startup

96661e9

pg_history_service: Fix LATEST logic

984de4d

progval commented Oct 27, 2024

View reviewed changes

sable_ircd/src/command/handlers/chathistory.rs Show resolved Hide resolved

progval added 7 commits October 27, 2024 21:17

Make sure server-time is consistent between echoes and CHATHISTORY

d56db83

Update down.sql

71ae9c1

Split make_historical_event out of get_entries

b87c630

Implement BEFORE/AFTER/BETWEEN

82d4546

Implement TieredHistoryService::list_targets

60a6fac

Merge branch 'master' into remote-history-service

3859cf8

Finish implementing TieredHistoryService

bdcc1e6

spb reviewed Nov 2, 2024

View reviewed changes

progval added 3 commits November 2, 2024 13:44

Remove unused dependency on async-trait

89143da

Remove unused enum HistoryMessage

0b245bf

Remove panics

61f78eb

spb reviewed Nov 9, 2024

View reviewed changes

progval added 2 commits November 9, 2024 15:45

Deduplicate query collection

a8f711b

Implement Around

ffd8f83

progval marked this pull request as ready for review November 10, 2024 18:16

progval changed the title ~~[WIP] Query history from history server~~ Query history from history server Nov 10, 2024

spb merged commit ede3e68 into Libera-Chat:master Nov 17, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query history from history server #143

Query history from history server #143

progval commented Oct 26, 2024 •

edited

Loading

progval commented Nov 2, 2024 •

edited

Loading

spb Nov 2, 2024

progval Nov 2, 2024

spb Nov 2, 2024

progval Nov 2, 2024

progval Nov 2, 2024

spb Nov 2, 2024

spb Nov 2, 2024

spb Nov 2, 2024

spb Nov 9, 2024

progval Nov 9, 2024 •

edited

Loading

progval Nov 9, 2024

spb Nov 9, 2024

progval Nov 9, 2024

spb Nov 9, 2024

progval Nov 9, 2024

spb Nov 9, 2024

progval Nov 9, 2024


		let mut connection_lock = self.database_connection.lock().await;

		let db_channel_id = i64::try_from(channel_id.as_u64()).expect("channel id overflows u64");

Query history from history server #143

Query history from history server #143

Conversation

progval commented Oct 26, 2024 • edited Loading

progval commented Nov 2, 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

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

progval Nov 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

progval commented Oct 26, 2024 •

edited

Loading

progval commented Nov 2, 2024 •

edited

Loading

progval Nov 9, 2024 •

edited

Loading