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

chore: refactor hot tags endpoint #294

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

chore: refactor hot tags endpoint #294

wants to merge 16 commits into from

Conversation

tipogi
Copy link
Collaborator

@tipogi tipogi commented Jan 17, 2025

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 test.
  • Performance: Ensure new code has relevant performance benchmarks, cargo bench

@tipogi tipogi self-assigned this Jan 17, 2025
@tipogi tipogi marked this pull request as ready for review January 17, 2025 11:30
src/db/graph/queries/get.rs Outdated Show resolved Hide resolved
WITH DISTINCT reach, MAX(tag.indexed_at) AS latest_tag_time
ORDER BY latest_tag_time DESC

// Use slice notation instead of SKIP and LIMIT
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a reason for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, if we use SKIP and LIMIT together, the SKIP clause does not work as it has to. That's why I went with slice notation, it is not the optimal but it works

Copy link
Collaborator

@amirRamirfatahi amirRamirfatahi Jan 21, 2025

Choose a reason for hiding this comment

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

yeah the original question meant to ask why wouldn't skip and limit work?

src/db/kv/traits.rs Outdated Show resolved Hide resolved
src/routes/v0/tag/global.rs Outdated Show resolved Hide resolved
@@ -598,16 +672,18 @@ async fn test_hot_tags_by_friends_reach_and_all_timeframe() -> Result<()> {

const PUBKY_TAG: &str = "pubky";

// KEEP ON EYE in the list. Sometimes it changes the order. No reason
Copy link
Collaborator

Choose a reason for hiding this comment

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

this isn't really good. If the order changes we have two options:

  1. Fix the database queries to make sure the order is set so it's not random.
  2. In the test, evaluate the list in a way that the order won't matter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

and this is what it is done, test_hot_tags_label_taggers make two request because the list was changing the order

Copy link
Collaborator

Choose a reason for hiding this comment

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

how's that going to solve it? Also why does it do 2 requests? First one seems to not pass params but the second one is passing a skip, limit?
I don't understand why.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore (service/watcher): refactor hot tags caching and remove legacy code
2 participants