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

Count team members using es #1046

Merged
merged 16 commits into from
Apr 9, 2020
Merged

Conversation

akshaymankar
Copy link
Member

@akshaymankar akshaymankar commented Apr 6, 2020

@akshaymankar akshaymankar force-pushed the akshaymankar/count-team-members-using-es branch from a17b33b to bdcbc00 Compare April 8, 2020 09:18
@akshaymankar akshaymankar marked this pull request as ready for review April 8, 2020 11:35
@akshaymankar akshaymankar changed the title [WIP] Count team members using es Count team members using es Apr 8, 2020
@akshaymankar
Copy link
Member Author

Whoops, didn't expect spar to break. Fixing it now.

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.

I have read everything but the integration tests. Looks good so far, but it has conflicts with #1029.

services/galley/src/Galley/API/Teams.hs Outdated Show resolved Hide resolved
let sizeAfterDelete =
if sizeBeforeDelete == 0
then 0
else sizeBeforeDelete - 1
Copy link
Contributor

Choose a reason for hiding this comment

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

hum, this does not inspire a lot of trust... but i have no idea how to make this better. :(

so if the same thing happens between 1809 and 1810 instead of 0 and 1, the result will be off by one?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand what you mean by 1809 and 1810. Can you please explain?

Copy link
Contributor

Choose a reason for hiding this comment

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

i mean that if you run into this race condition with number higher than 1 and 0, this will just silently get the count wrong by 1.

Does this accumulate? So every time we enter the count becomes less accurate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this can be off by more than 1, if a lot of people join/leave the team within a very short time (seconds), we are bound to get this number wrong.

services/galley/src/Galley/Intra/Journal.hs Show resolved Hide resolved
Default to 1 if ES says team has 0 users.
@fisx fisx merged commit 20e1609 into develop Apr 9, 2020
@fisx fisx deleted the akshaymankar/count-team-members-using-es branch April 9, 2020 20:39
@fisx fisx mentioned this pull request Apr 21, 2020
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.

None yet

2 participants