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

Use maxShards instead of lastShardID #1339

Merged
merged 1 commit into from
Mar 9, 2022
Merged

Use maxShards instead of lastShardID #1339

merged 1 commit into from
Mar 9, 2022

Conversation

GweeKwee
Copy link
Contributor

If we spawn a client with a defined range of shards, a guild may point to the wrong shard id.

DonovanDMC
DonovanDMC approved these changes Feb 11, 2022
View reviewed changes
Copy link
Contributor

@DonovanDMC DonovanDMC left a comment

Choose a reason for hiding this comment

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

I've never noticed any issues, but an issue happening here would make sense.

bsian03
bsian03 approved these changes Mar 9, 2022
View reviewed changes
Copy link
Collaborator

@bsian03 bsian03 left a comment

Choose a reason for hiding this comment

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

untested, but lgtm

@curtisf curtisf merged commit c1f1f86 into abalabahaha:dev Mar 9, 2022
@Dramex
Copy link
Contributor

Dramex commented Mar 9, 2022

What if the client doesn’t have all the shards (aka cluster) is this still gonna work? 🤔

@DonovanDMC
Copy link
Contributor

DonovanDMC commented Mar 9, 2022 •
edited
Loading

That's the problem this fixes
maxShards is passed on to Discord on all gateway sessions, so guild ids should be based off of the total number of shards overall, not what's just present in the current process/cluster

That shouldn't be a problem since each process will only get the guilds for the shards they have

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsian03 bsian03 bsian03 approved these changes

@curtisf curtisf curtisf approved these changes

@DonovanDMC DonovanDMC DonovanDMC approved these changes

Assignees
No one assigned
Labels
enhancement potential bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@GweeKwee @Dramex @DonovanDMC @curtisf @bsian03