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.

Sign up for GitHub

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

Jump to bottom

Move search operations to UserSubsystem #4188

Merged
merged 53 commits into from
Sep 19, 2024
Merged

Move search operations to UserSubsystem #4188

merged 53 commits into from
Sep 19, 2024

Conversation

akshaymankar
Copy link
Member

@akshaymankar akshaymankar commented Aug 6, 2024 •
edited by mdimjasevic
Loading

https://wearezeta.atlassian.net/browse/WPB-8888

Checklist

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

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Aug 6, 2024
@akshaymankar akshaymankar force-pushed the wpb-8888/search-subsystem branch 5 times, most recently from f8ceeb9 to d765d9a Compare August 12, 2024 14:36
@akshaymankar akshaymankar force-pushed the wpb-8888/search-subsystem branch 7 times, most recently from 51287eb to 1273d16 Compare August 20, 2024 07:25
akshaymankar
akshaymankar commented Aug 20, 2024
View reviewed changes
libs/wire-subsystems/src/Wire/IndexedUserStore/ElasticSearch.hs
script :: ES.Script
script = ES.Script (Just (ES.ScriptLanguage "painless")) (Just (ES.ScriptInline scriptText)) Nothing Nothing

-- Unfortunately ES disallows updating ctx._version with a "Update By Query"
Copy link
Member Author

Choose a reason for hiding this comment

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

Not updating versions can cause issues while reindexing. This seems like an oversight, we should try to break it and create tickets accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't block this PR.

@akshaymankar akshaymankar force-pushed the wpb-8888/search-subsystem branch 4 times, most recently from 9ab91b3 to 14fa9dc Compare August 21, 2024 15:17
@akshaymankar
UserStore.Cassandra: Dedup embed call
aa014bf
@akshaymankar
Move all indexing operations to UserSearchSubsystem
cac29c2 
Pending:
- Index Management
- Search
- Metrics
- Update brig to use the new code  (currently, brig is just broken)
@akshaymankar
Add metrics to UserSearchSubsystem and IndexedUserStore
082fdcd 
Some metrics have been deleted as they are for bulk operations and there is no
way for us to get those metrics because these operations don't run in an http
request.
@akshaymankar
Run user serach index data migrations using subsystems
aac6ed0
@akshaymankar
Remove explicit creds from ESConn
c234972
@echoes-hq echoes-hq bot added the echoes: throughput Changes intended at preserving our ability to evolve the software safely and effectively label Sep 17, 2024
@elland
Resolve todo:
1806e82 
Moved a function, drive-by clean-up of lenses overuse.
@elland elland marked this pull request as ready for review September 17, 2024 13:48
@akshaymankar akshaymankar changed the title Introduce Search Subsystem Move search operations to UserSubsystem Sep 17, 2024
@echoes-hq echoes-hq bot added the echoes: technical-debt Changes intended at mitigating risks label Sep 17, 2024
akshaymankar
akshaymankar commented Sep 17, 2024
View reviewed changes
libs/wire-api/src/Wire/API/Error/Brig.hs Outdated
Comment on lines 174 to 175
type instance MapError 'NoUser = 'StaticError 403 "no-user" "The user does not exist"

Copy link
Member Author

Choose a reason for hiding this comment

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

What was the error before the refactoring?

Copy link
Contributor

Choose a reason for hiding this comment

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

"TODO: searcher doesn't exist" :)

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 was asking about before this PR started. This PR is supposed to be a refactoring, so ideally we wouldn't be inventing new user visible errors. So it'd be great to know what was the previous behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like we've introduced this error, but for no good reason. I'm refactoring the code so we don't need to throw the error.

Copy link
Contributor

Choose a reason for hiding this comment

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

This type instance is gone now and we don't throw a new error anymore compared to the old code.

akshaymankar
akshaymankar commented Sep 17, 2024
View reviewed changes
libs/wire-subsystems/src/Wire/FederationConfigStore.hs Outdated
Comment on lines 27 to 28
-- TODO: This store effect is more than just a store, we should break it up in
-- business logic and store
-- FUTUREWORK: This store effect is more than just a store,
-- we should break it up in business logic and store
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this still work for this ticket? We moved the code from brig to wire-subsystems in this PR so it might be confusing for someone to see that it wasn't made to fit the nomenclature here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still work for this ticket? We moved the code from brig to wire-subsystems in this PR so it might be confusing for someone to see that it wasn't made to fit the nomenclature here.

Is this a blocker for you or is a follow-up ticket OK?

Copy link
Member Author

Choose a reason for hiding this comment

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

Follow up is also ok 👍

mdimjasevic
mdimjasevic reviewed Sep 18, 2024
View reviewed changes
Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

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

This is just a partial review in case anyone has capacity to address my comments while I review the rest of the PR.

libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs
Comment on lines +143 to +144
UpdateTeamSearchVisibilityInbound status ->
updateTeamSearchVisibilityInboundImpl status
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this one be in a team effect?

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a blocker for me.

libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs
Comment on lines +147 to +148
BrowseTeam uid browseTeamFilters mMaxResults mPagingState ->
browseTeamImpl uid browseTeamFilters mMaxResults mPagingState
Copy link
Contributor

Choose a reason for hiding this comment

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

This one too sounds like it belongs to a team effect.

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a blocker for me.

Copy link
Member Author

@akshaymankar akshaymankar Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Makes sense, but I think this would make a very weird first thing to implement in a TeamSubsystem. Should we add a note here and move it later? Same for UpdateTeamSearchVisibilityInbound.

libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs
@@ -397,6 +434,8 @@ updateUserProfileImpl (tUnqualified -> uid) mconn updateOrigin update = do
guardLockedFields user updateOrigin update
mapError (\StoredUserUpdateHandleExists -> UserSubsystemHandleExists) $
updateUser uid (storedUserUpdate update)
let interestingToUpdateIndex = isJust update.name || isJust update.accentId
Copy link
Contributor

Choose a reason for hiding this comment

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

Why only these two are relevant here, and not others? Somewhere else we have a helper function that computes for an update if it is interesting. How do these two relate?

libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs Outdated Show resolved Hide resolved
libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs Outdated Show resolved Hide resolved
libs/wire-subsystems/test/unit/Wire/MockInterpreters/FederationConfigStore.hs Outdated Show resolved Hide resolved
libs/wire-subsystems/test/unit/Wire/MockInterpreters/UserStore.hs Outdated Show resolved Hide resolved
services/brig/src/Brig/API/Internal.hs
liftSem $
if success
then do
UserSubsystem.internalUpdateSearchIndex uid
Copy link
Contributor

@mdimjasevic mdimjasevic Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Was something like this done in the version before this PR?

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 think Intra.onUserEvent did this.

services/brig/src/Brig/API/User.hs
@@ -335,7 +328,7 @@ createUser new = do

wrapClient $ Data.insertAccount account Nothing pw False
liftSem $ GalleyAPIAccess.createSelfConv uid
liftSem $ Intra.onUserEvent uid Nothing (UserCreated (accountUser account))
liftSem $ Events.generateUserEvent uid Nothing (UserCreated (accountUser account))
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to update the search index here?

If it needed, why did we pull out this updating? It just opens room for errors without any clear benefits to me.

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 think activateUser does that part.

The reason for pulling this out was that we were processing notifications meant for users as if they were intra backend communication which didn't seem very intuitive. Yes, this opens a room for bugs, but we have to trust our testing otherwise refactorings like this one would never happen.

services/brig/src/Brig/API/User.hs Show resolved Hide resolved
mdimjasevic
mdimjasevic approved these changes Sep 18, 2024
View reviewed changes
Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

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

There are still a few things left to address, and I've pointed to them in my earlier reviews. Overall, this looks good, but please don't merge before addressing the remaining issues.

services/brig/src/Brig/Team/API.hs Outdated Show resolved Hide resolved
services/brig/src/Brig/Team/API.hs Outdated Show resolved Hide resolved
services/brig/src/Brig/Team/API.hs Outdated Show resolved Hide resolved
@mdimjasevic mdimjasevic merged commit c04e583 into develop Sep 19, 2024
9 checks passed
@mdimjasevic mdimjasevic deleted the wpb-8888/search-subsystem branch September 19, 2024 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elland elland elland left review comments

@mdimjasevic mdimjasevic mdimjasevic approved these changes

Assignees
No one assigned
Labels
echoes: technical-debt Changes intended at mitigating risks echoes: throughput Changes intended at preserving our ability to evolve the software safely and effectively 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
@akshaymankar @elland @mdimjasevic @zebot