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

Fix fetching list of channels #491

Closed
wants to merge 1 commit into from
Closed

Fix fetching list of channels #491

wants to merge 1 commit into from

Conversation

hloeung
Copy link
Collaborator

@hloeung hloeung commented Nov 5, 2022

In addition to #490, this fixes the retrieval of the list of the list of channels.

Per #243 & #248, the Mattermost server has a hardcoded maximum page limit of 200. So when adding debugging output, I get:

[2022-11-05T13:54:39+11:00]  INFO matterclient: found 158 channels for user in team canonical
[2022-11-05T13:54:39+11:00]  INFO matterclient: found 200 public channels in team canonical

With this, we get higher numbers:

[2022-11-05T14:14:50+11:00]  INFO matterclient: found 158 channels for user in team canonical
[2022-11-05T14:14:55+11:00]  INFO matterclient: found 1116 public channels in team canonical

This is also what's causing #479

@hloeung hloeung mentioned this pull request Nov 5, 2022
Merged
@hloeung
Copy link
Collaborator Author

hloeung commented Nov 5, 2022

There are other places hardcoding page 0 with a larger page limit (5k and 50k) but that can be addressed in future PRs.

@hloeung hloeung changed the title Fix fetching list of channels [WIP] Fix fetching list of channels Nov 5, 2022
@hloeung hloeung marked this pull request as ready for review November 5, 2022 06:44
@hloeung hloeung changed the title [WIP] Fix fetching list of channels Fix fetching list of channels Nov 5, 2022
hloeung
hloeung commented Nov 5, 2022
View reviewed changes
mm-go-irckit/userbridge.go
@@ -808,7 +808,7 @@ func (u *User) addUserToChannelWorker6(channels <-chan *bridge.ChannelInfo, thro
if postlist == nil {
// if the channel is not from the primary team id, we can't get posts
if brchannel.TeamID == u.br.GetMe().TeamID {
logger.Errorf("something wrong with getPostsSince for %s for channel %s (%s)", u.Nick, brchannel.ID, brchannel.Name)
logger.Errorf("something wrong with getPostsSince for %s for channel %s (%s)", u.Nick, channame, brchannel.ID)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Drive by fix, scroll up 20 lines to 788

@hloeung
Copy link
Collaborator Author

hloeung commented Nov 5, 2022

This is mostly copied from the fix for users in #244 / 8e043ed

42wim added a commit to matterbridge/matterclient that referenced this pull request Nov 5, 2022
@42wim
Paginate channels to get over 200 channels limit
bf1c6ad 
Based upon 42wim/matterircd#491
@42wim 42wim mentioned this pull request Nov 5, 2022
Merged
@42wim
Copy link
Owner

42wim commented Nov 5, 2022

@hloeung could you try with matterbridge/matterclient#1 (based upon your code)
Works fine for me, but test it before I merge :)

@hloeung
Copy link
Collaborator Author

hloeung commented Nov 5, 2022

@42wim, Sure! Trying that now.

@hloeung
Copy link
Collaborator Author

hloeung commented Nov 5, 2022

@42wim LGTM. Also cleaner fix, thanks.

One thing, can we make the found X messages INFO?

[2022-11-05T21:04:16Z]  INFO matterclient: Found version 6.6.0.6.6.0.96a259e895e1143a8e843b070c21019a.true
[2022-11-05T21:04:21Z] DEBUG matterclient: found 947 users in team canonical
[2022-11-05T21:04:21Z] DEBUG matterclient: initUser(): found our team canonical (id: sqmc4sz45prypmkfctwynm5yjr)
[2022-11-05T21:04:26Z] DEBUG matterclient: found 158 channels for user in team canonical
[2022-11-05T21:04:26Z] DEBUG matterclient: found 1316 public channels in team canonical

I think those are useful or nice to see.

@hloeung
Copy link
Collaborator Author

hloeung commented Nov 5, 2022

Oh, also FWIW, on working on this, I noticed quite a few calls to UpdateChannelsTeam(). With this change, it means even more requests due to pagination. Maybe something to consider for optimising and improving next.

@42wim
Copy link
Owner

42wim commented Nov 5, 2022

wrt Info vs Debug, it's very subjective of course, but I think it's more debug info than actual needed info.
On the fence though :)

@hloeung
Copy link
Collaborator Author

hloeung commented Nov 5, 2022

heh no problem. Filed #492 for the frequency of these calls

42wim added a commit to matterbridge/matterclient that referenced this pull request Nov 6, 2022
@42wim
Paginate channels to get over 200 channels limit (#1)
e65d60f 
Based upon 42wim/matterircd#491
@42wim
Copy link
Owner

42wim commented Nov 6, 2022

Superseded by #494

@42wim 42wim closed this Nov 6, 2022
@hloeung hloeung deleted the fix-fetching-channels branch November 6, 2022 19:54
@hloeung hloeung mentioned this pull request Feb 13, 2024
Open
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
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@hloeung @42wim