Skip to content

[SQSERVICES-1828] pagination of team members does not work properly for certain team sizes#2968

Merged
battermann merged 11 commits intodevelopfrom
SQSERVICES-1828-be-pagination-of-team-members-does-not-work-properly-for-certain-team-sizes
Jan 13, 2023
Merged

[SQSERVICES-1828] pagination of team members does not work properly for certain team sizes#2968
battermann merged 11 commits intodevelopfrom
SQSERVICES-1828-be-pagination-of-team-members-does-not-work-properly-for-certain-team-sizes

Conversation

@battermann
Copy link
Contributor

@battermann battermann commented Jan 9, 2023 •
edited
Loading

https://wearezeta.atlassian.net/browse/SQSERVICES-1828

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@battermann battermann temporarily deployed to cachix January 9, 2023 16:11 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix January 9, 2023 16:11 — with GitHub Actions Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Jan 9, 2023
@battermann battermann temporarily deployed to cachix January 10, 2023 07:35 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix January 10, 2023 07:35 — with GitHub Actions Inactive
@battermann battermann requested a review from fisx January 10, 2023 07:35
@battermann battermann marked this pull request as ready for review January 10, 2023 07:35
@battermann battermann removed the request for review from fisx January 10, 2023 07:40
@battermann battermann marked this pull request as draft January 10, 2023 07:40
elland
elland reviewed Jan 10, 2023
View reviewed changes
@battermann battermann temporarily deployed to cachix January 10, 2023 09:28 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix January 10, 2023 09:28 — with GitHub Actions Inactive
@battermann battermann marked this pull request as ready for review January 10, 2023 09:29
@battermann battermann requested a review from fisx January 10, 2023 09:29
@battermann battermann force-pushed the SQSERVICES-1828-be-pagination-of-team-members-does-not-work-properly-for-certain-team-sizes branch from ae96ae1 to a741fba Compare January 10, 2023 09:30
fisx
fisx reviewed Jan 10, 2023
View reviewed changes
services/brig/src/Brig/User/Search/TeamUserSearch.hs
where

sortingTieBreaker :: [ES.DefaultSort]
sortingTieBreaker = [ES.DefaultSort (ES.FieldName "_doc") ES.Ascending Nothing Nothing Nothing Nothing]
Copy link
Contributor

@fisx fisx Jan 10, 2023

Choose a reason for hiding this comment

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

I'm not sure, but this looks like something different from what the docs you are linking suggest, and it may be prohibitively inefficient.

Can you try the _id trick that you were about to try? That may require an ES migration, but we've done those before, and ES doesn't server as a source of truth and in the worst case can recover from Cassandra. So I'm not too worried.

@battermann @fisx
Update changelog.d/3-bug-fixes/pr-2968
a0f3b8c 
Co-authored-by: fisx <mf@zerobuzz.net>
@battermann battermann temporarily deployed to cachix January 10, 2023 10:41 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix January 10, 2023 10:41 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix January 10, 2023 10:41 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix January 10, 2023 10:42 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix January 10, 2023 10:42 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix January 10, 2023 12:26 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix January 10, 2023 12:26 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix January 10, 2023 13:44 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix January 10, 2023 13:44 — with GitHub Actions Inactive
@battermann battermann requested a review from fisx January 10, 2023 13:44
@battermann battermann temporarily deployed to cachix January 10, 2023 13:54 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix January 10, 2023 13:54 — with GitHub Actions Inactive
fisx
fisx reviewed Jan 11, 2023
View reviewed changes
services/brig/test/integration/API/Search.hs Outdated
Copy link
Contributor

@fisx fisx Jan 11, 2023

Choose a reason for hiding this comment

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

I can't parse (or understand) this sentence.

@battermann battermann force-pushed the SQSERVICES-1828-be-pagination-of-team-members-does-not-work-properly-for-certain-team-sizes branch from 362f3e6 to b19a3b3 Compare January 12, 2023 09:45
@battermann battermann temporarily deployed to cachix January 12, 2023 09:45 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix January 12, 2023 09:45 — with GitHub Actions Inactive
@battermann battermann requested a review from fisx January 12, 2023 09:47
@battermann battermann temporarily deployed to cachix January 12, 2023 10:12 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix January 12, 2023 10:12 — with GitHub Actions Inactive
fisx
fisx approved these changes Jan 12, 2023
View reviewed changes
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

maybe you can add something to the swagger docs of the /search endpoint along the lines of "if you can, use /list-users, /search is more powerful but potentially more expensive and less reliable"? not sure.

@battermann
Copy link
Contributor Author

battermann commented Jan 12, 2023

maybe you can add something to the swagger docs of the /search endpoint along the lines of "if you can, use /list-users, /search is more powerful but potentially more expensive and less reliable"? not sure.

But is that really true?

@fisx
Copy link
Contributor

fisx commented Jan 12, 2023

ci seems in trouble:

        Exception: addWatch: resource exhausted (No space left on device)

@battermann battermann temporarily deployed to cachix January 12, 2023 15:31 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix January 12, 2023 15:31 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix January 12, 2023 16:24 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix January 12, 2023 16:24 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix January 13, 2023 09:23 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix January 13, 2023 09:23 — with GitHub Actions Inactive
@battermann battermann merged commit d83c679 into develop Jan 13, 2023
@battermann battermann deleted the SQSERVICES-1828-be-pagination-of-team-members-does-not-work-properly-for-certain-team-sizes branch January 13, 2023 12:28
@zebot zebot mentioned this pull request Jan 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fisx fisx fisx approved these changes

+1 more reviewer

@elland elland elland left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@battermann @fisx @elland @zebot

Comments