Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Add watcher support indexing multiple homeservers #314

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

amirRamirfatahi
Copy link
Collaborator

Fixes #200 and #209

Pre-submission Checklist

For tests to work you need a working neo4j and redis instance with the example dataset in docker/db-graph

  • Testing: Implement and pass new tests for the new features/fixes, cargo nextest run.
  • Performance: Ensure new code has relevant performance benchmarks, cargo bench

@amirRamirfatahi amirRamirfatahi self-assigned this Feb 4, 2025
@amirRamirfatahi amirRamirfatahi changed the base branch from main to feat/retry-manager February 4, 2025 02:30
@amirRamirfatahi amirRamirfatahi marked this pull request as ready for review February 4, 2025 03:48
@amirRamirfatahi amirRamirfatahi marked this pull request as draft February 4, 2025 03:49
@@ -309,3 +310,20 @@ pub fn create_file(file: &FileDetails) -> Result<Query, DynError> {

Ok(query)
}

// Create a Homeserver node
pub fn create_homeserver(homeserver: &Homeserver) -> Result<Query, DynError> {
Copy link
Collaborator

@SHAcollision SHAcollision Feb 4, 2025

Choose a reason for hiding this comment

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

I thought about storing Homeserver into Neo4J also when I implemented it. I think the main issue with of storing Homeserver in the graph is a possible dataloss of Redis indexes on crash.

In case of restart/crash, we might restore the cursor from graph instead of Redis. Redis last snapshot might not be the latest cursor on graph, therefore, we will resume watching homeservers and skip some events.

I'm sure we can mitigate this easy by only restoring Homeserver from Redis on watcher startup, without fallback to graph (or only on some conditions).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm...I don't know if I understood that correctly or not, but I'm also updating the cursor in graph?

Copy link
Collaborator

@SHAcollision SHAcollision Feb 5, 2025

Choose a reason for hiding this comment

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

Yes, and that's exactly the problem: the cursor in graph might be newer than the latest synced events in Redis in case of a restart/crash.

In other words, our assumption has always been that using Redis without persistence is OK and safe if and only if when we lose Redis data we also lose the cursor. This way, in the next startup, the lost events will be re-acquired. However, saving the cursor to graph breaks this guarantee.

Base automatically changed from feat/retry-manager to main February 4, 2025 14:52
@amirRamirfatahi amirRamirfatahi changed the base branch from main to feat/retry-manager-mpsc February 4, 2025 21:27
@amirRamirfatahi amirRamirfatahi changed the base branch from feat/retry-manager-mpsc to main February 4, 2025 21:33
@amirRamirfatahi amirRamirfatahi changed the base branch from main to feat/retry-manager-mpsc February 4, 2025 21:41
@amirRamirfatahi amirRamirfatahi force-pushed the feature/multi-homeserver-watcher-support branch from b2b7d7c to 37b8c07 Compare February 4, 2025 22:36
@amirRamirfatahi amirRamirfatahi changed the base branch from feat/retry-manager-mpsc to main February 4, 2025 22:36
@SHAcollision SHAcollision mentioned this pull request Feb 5, 2025
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feat (dex): permanently store homeservers as graph nodes
2 participants