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

Fixes #24779: Groups compliance summary need API pagination #5635

Draft
wants to merge 1 commit into
base: branches/rudder/8.1
Choose a base branch
from

Conversation

clarktsiory
Copy link
Contributor

@clarktsiory clarktsiory commented Apr 26, 2024

https://issues.rudder.io/issues/24779

Some decisions :

  • we don't do server-side sorting and pagination with LDAP which seems overkill here (we only have id, name, category and we still would have to handle it for compliance which is computed in-memory)
  • we do pagination in-memory, without any cache system for now
  • we sort compliance by computing all group compliance first and then sorting naively

The changes :

  • Introducing a QueryFilter case class for the pagination logic
  • Rewriting the group repository method to get the groups, because we want to also get the group category to sort it
  • TODO : optimize sort by compliance : we currently compute both targeted and global compliance on an individual group (horizontally), but we want to only compute the whole column for the sorted one (vertically)

Later improvements

Caching would be ideal but is hard if we don't know when to evict the cache : a change on any group and on policy generation and maybe something else...

@clarktsiory clarktsiory requested a review from fanf April 26, 2024 14:11
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

@clarktsiory clarktsiory marked this pull request as draft April 26, 2024 14:13
@clarktsiory
Copy link
Contributor Author

PR rebased

@clarktsiory clarktsiory force-pushed the bug_24779/groups_compliance_summary_need_api_pagination branch from 8414dfd to 499284d Compare September 10, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant