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

Support saving last viewed at state for multiple users #425

Merged
merged 2 commits into from
Nov 11, 2021
Merged

Support saving last viewed at state for multiple users #425

merged 2 commits into from
Nov 11, 2021

Conversation

hloeung
Copy link
Collaborator

@hloeung hloeung commented Apr 8, 2021

matterircd supports multiple users so we save separate last viewed at state files for each user. We also only load them after a user has logged in rather than on start up.

@42wim
Copy link
Owner

42wim commented Apr 14, 2021

I'm thinking that putting the info in 1 file instead of a file for each user is cleaner? WDYT ?

@42wim 42wim added this to the 0.24.1 milestone Apr 14, 2021
@hloeung
Copy link
Collaborator Author

hloeung commented Apr 22, 2021

I'm thinking that putting the info in 1 file instead of a file for each user is cleaner? WDYT ?

But then it would have to keep a global data structure instead of in NewUserBridge() and synchronize between the go routines when writing / saving the state file, IIUC.

@42wim
Copy link
Owner

42wim commented Apr 28, 2021

I don't think it's a good idea to have multiple state files, we can do this in one file :-)
but yes, you'd have some more locking issues then, you should only have one writer that writes the file using channels.

But as we're putting more stuff on disk what do you think about using boltdb as k/v store for this instead. You can instantiate one global writer so you don't need to do the locking yourself.

@42wim 42wim modified the milestones: 0.24.1, 0.24.2 Apr 29, 2021
@hloeung
Copy link
Collaborator Author

hloeung commented Apr 30, 2021

Ah sure, I'll take a look at boltdb or something similar so that state files for multiple users are all contained within the one file on disk.

@hloeung hloeung marked this pull request as draft April 30, 2021 04:20
@42wim 42wim modified the milestones: 0.24.2, 0.24.3 Jun 13, 2021
@hloeung hloeung force-pushed the persistent-last-viewed-multi-users branch 2 times, most recently from 9c93ffb to 26c73ba Compare June 27, 2021 04:21
@hloeung
Support saving last viewed at state for multiple users
ff08c53 
Persistent last viewed at state was added in PR 42wim#313. It is done by
dumping out the state of the current user to a file on
disk. Unfortunately, this is only per user, the current user, even
though matterircd supports multiple users.

This fixes that by switching to using bbolt for storing and saving the
last viewed at state.
@hloeung hloeung force-pushed the persistent-last-viewed-multi-users branch from 26c73ba to ff08c53 Compare June 27, 2021 04:30
@hloeung
Copy link
Collaborator Author

hloeung commented Jun 27, 2021 •
edited
Loading

@42wim , converted to using Bolt. It's also backwards compatible loading the existing values and then cleaning it up:

INFO[2021-06-27T14:28:44+10:00] Running version 0.24.3-dev                    module=matterircd
INFO[2021-06-27T14:28:44+10:00] WARNING: THIS IS A DEVELOPMENT VERSION. Things may break.  module=matterircd
WARN[2021-06-27T14:28:44+10:00] Found old last viewed at state file, renaming to .migrated  module=matterircd
INFO[2021-06-27T14:28:44+10:00] Listening on 127.0.0.1:6697                   module=matterircd
INFO[2021-06-27T14:28:46+10:00] New connection: 127.0.0.1:49584               module=matterircd
loggerlevel: info
INFO[2021-06-27T14:29:01+10:00] login as hloeung (team: myteam) on mymattermostserver.local
[2021-06-27T14:29:02+10:00]  INFO matterclient: Found version 5.33.3.5.33.3.41edc0d55bd344e85da75fededb08bee.true
[2021-06-27T14:29:05+10:00]  INFO matterclient: found 686 users in team myteam
INFO[2021-06-27T14:29:07+10:00] login succeeded
WARN[2021-06-27T14:29:07+10:00] Found old last viewed at state file, loading and removing  module=matterircd
INFO[2021-06-27T14:29:07+10:00] Loaded lastViewedAt from 2021-06-26 09:05:47 +1000 AEST  module=matterircd
INFO[2021-06-27T14:29:16+10:00] Replaying msgs for hloeung for #channel1 (okgji...) since 2021-06-26 09:18:34 (stored)  module=matterircd
INFO[2021-06-27T14:29:17+10:00] Replaying msgs for hloeung for #channel2 (1tq37...) since 2021-06-26 10:28:19 (stored)  module=matterircd
INFO[2021-06-27T14:29:17+10:00] Replaying msgs for hloeung for #channel3 (ymnb5...) since 2021-06-26 10:06:49 (stored)  module=matterircd
INFO[2021-06-27T14:29:18+10:00] Replaying msgs for hloeung for #channel4 (64ynn...) since 2021-06-26 09:14:24 (stored)  module=matterircd
INFO[2021-06-27T14:29:21+10:00] Replaying msgs for hloeung for #channel5 (uw577...) since 2021-06-26 10:45:26 (server)  module=matterircd
INFO[2021-06-27T14:29:22+10:00] Replaying msgs for hloeung for #channel6 (53bct...) since 2021-06-26 17:33:18 (stored)  module=matterircd
INFO[2021-06-27T14:29:22+10:00] Replaying msgs for hloeung for #channel7 (c38ws...) since 2021-06-26 10:07:03 (stored)  module=matterircd

@hloeung hloeung marked this pull request as ready for review June 27, 2021 04:33
@hloeung hloeung force-pushed the persistent-last-viewed-multi-users branch from dd62397 to ff08c53 Compare June 27, 2021 06:34
@hloeung hloeung mentioned this pull request Jun 27, 2021
Merged
@42wim 42wim merged commit 45d6e01 into 42wim:master Nov 11, 2021
@42wim
Copy link
Owner

42wim commented Nov 11, 2021

Seems I overlooked this PR :(
Thanks for your work @hloeung 👍

@hloeung
Copy link
Collaborator Author

hloeung commented Nov 11, 2021

Thank you @42wim !

@hloeung hloeung deleted the persistent-last-viewed-multi-users branch November 11, 2021 23:10
@hloeung hloeung mentioned this pull request Oct 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.25.0
Development

Successfully merging this pull request may close these issues.
2 participants
@hloeung @42wim