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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only sync at most 250 channels at the same time #3188

Merged

Conversation

nuno-vieira
Copy link
Member

@nuno-vieira nuno-vieira commented May 9, 2024

馃敆 Issue Links

Resolves https://github.com/GetStream/ios-issues-tracking/issues/792

馃幆 Goal

Fixes trying to sync more than 255 channels at the same time. (Backend limit)

馃摑 Summary

Changes the limit to 250, and it should be more than enough. At most, customers will have about 100 active channels at the same time, and even then, it wouldn't be a big deal if all channels where not synced. Only the most recents are important, and eventually, they will be refetched anyways.

The reason we could hit 255+ channels syncing is that we never erase the channels from the DB; we just unlink them from the queries. So, there could be a case where a user could have more than 255 channels in their local DB, with really old channels.

馃И Manual Testing Notes

N/A

鈽戯笍 Contributor Checklist

  • I have signed the Stream CLA (required)
  • This change follows zero 鈿狅笍 policy (required)
  • This change should be manually QAed
  • Changelog is updated with client-facing changes
  • New code is covered by unit tests
  • Comparison screenshots added for visual changes
  • Affected documentation updated (docusaurus, tutorial, CMS)

@nuno-vieira nuno-vieira added 馃悶聽Bug An issue or PR related to a bug 馃寪聽SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK labels May 9, 2024
@nuno-vieira nuno-vieira requested a review from a team as a code owner May 9, 2024 15:43
@Stream-iOS-Bot
Copy link
Collaborator

StreamChat XCMetrics

target metric benchmark branch performance status
MessageList Hitches total duration 10 ms 6.68 ms 33.2% 馃敿 馃煝
Duration 2.6 s 2.54 s 2.31% 馃敿 馃煝
Hitch time ratio 4 ms per s 2.63 ms per s 34.25% 馃敿 馃煝
Frame rate 79 fps 77.8 fps 1.52% 馃敿 馃煝
Number of hitches 1 0.8 20.0% 馃敿 馃煝
ChannelList Hitches total duration 12.5 ms 7.51 ms 39.92% 馃敿 馃煝
Duration 2.6 s 2.27 s 12.69% 馃敿 馃煝
Hitch time ratio 5 ms per s 3.28 ms per s 34.4% 馃敿 馃煝
Frame rate 76 fps 78.31 fps -3.04% 馃斀 馃煛
Number of hitches 1.2 0.6 50.0% 馃敿 馃煝

Copy link

sonarcloud bot commented May 9, 2024

@nuno-vieira nuno-vieira enabled auto-merge (squash) May 9, 2024 19:32
@@ -191,7 +191,7 @@ class SyncRepository {
private func getChannelIds(completion: @escaping ([ChannelId]) -> Void) {
database.backgroundReadOnlyContext.perform {
let request = ChannelDTO.allChannelsFetchRequest
request.fetchLimit = 1000
request.fetchLimit = 250
Copy link
Contributor

Choose a reason for hiding this comment

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

When looking at the code it seems like, if there are 500 channels, then we would only get 250 of them and the other 250 is never synced, only when opening it manually with a channel controller.

Another interesting thing with syncing is that it pauses all the other API requests until sync repo is done. Not sure why it is a requirement.

I guess what I am saying is that sync repo should need a review and the current change is nice!

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we say we will use 100? https://getstream.slack.com/archives/CRB9P6L03/p1712674377370179?thread_ts=1712674016.356819&cid=CRB9P6L03

Also, shouldn't we also cleanup/delete the rest of the channels?

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 did not notice that comment sorry. I also had Auto merge enabled, so the PR got merged when @laevandus approved 馃槃

Copy link
Member Author

Choose a reason for hiding this comment

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

The cleanup maybe better to make it part of a different PR, since it needs a strategy where we will do this

@nuno-vieira nuno-vieira merged commit 0518b0b into develop May 10, 2024
15 checks passed
@nuno-vieira nuno-vieira deleted the fix/syncing-more-than-250-channels-at-the-same-time branch May 10, 2024 06:18
@@ -7,6 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
### 馃悶 Fixed
- Fix triggering user query calls whenever a new user was added in `UserListController` [#3184](https://github.com/GetStream/stream-chat-swift/pull/3184)
- Fix duplicated watching channel calls when reconnecting [#3187](https://github.com/GetStream/stream-chat-swift/pull/3187)
- Fix `/sync` call failing because of surpassing the 255 channels limit [#3188](https://github.com/GetStream/stream-chat-swift/pull/3188)
Copy link
Contributor

Choose a reason for hiding this comment

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

250 channels limit

Copy link
Member Author

Choose a reason for hiding this comment

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

The limit is actually 255, not 250

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
馃悶聽Bug An issue or PR related to a bug 馃寪聽SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants