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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: added @discordjs/sharder #8859

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

kyranet
Copy link
Member

@kyranet kyranet commented Nov 24, 2022

Please describe the changes this PR makes and why it should be merged:

Supersedes #7204, implements part of #8084.

For more information about this sharder's architecture, please refer to the aforementioned RFC issue.

Features

  • ShardManager
    • Channel strategies (server)
      • Cluster
      • Fork (default)
      • WorkerThread
    • Events
      • Shard has been started
      • Shard has been restarted
      • Shard has been closed
      • Shard has signalled ready
      • Shard has signalled exit
      • Shard has sent an invalid message
      • Shard has sent a ping
      • Shard has not sent a ping
    • Ping
  • ShardClient
    • Channel strategies (client)
      • Cluster
      • Fork (default)
      • WorkerThread
    • Events
      • Ready
      • Exit
      • Starting
      • Restarting
  • Shared
    • Message strategies
      • JSON (default)
      • V8
    • Message timeout
    • Message abort
    • Message transformers
      • Gzip (will not be included in initial PR)
      • Brotli (will not be included in initial PR)
  • ShardManagerProxy (will not be included in initial PR)

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

@vercel
Copy link

vercel bot commented Nov 24, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Updated
discord-js ⬜️ Ignored (Inspect) Nov 24, 2022 at 3:25PM (UTC)
discord-js-guide ⬜️ Ignored (Inspect) Nov 24, 2022 at 3:25PM (UTC)

Copy link
Contributor

@DraftProducts DraftProducts left a comment

Choose a reason for hiding this comment

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

Nice supplant, excited to test cluster in production mode 🚀


public readonly spawnOptions: Required<ShardSpawnOptions>;

public readonly channels: IChannel[] = [];
Copy link
Contributor

@DraftProducts DraftProducts Nov 24, 2022

Choose a reason for hiding this comment

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

Perhaps we could choose another less conflicting name for IChannel & <ShardManager>.channels, it could be confused with traditional Discord channels, perhaps this is something to consider? Unless the fact that it's internal allows us to override this non-ambiguity rule?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's called channel because it's the... channel strategy both the manager and the clients use to communicate.

Copy link
Contributor

@DraftProducts DraftProducts Nov 24, 2022

Choose a reason for hiding this comment

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

Before the class that managed the shard like this (manager side) was called "shard", that why I was hesitant about this new name.

Copy link
Member Author

Choose a reason for hiding this comment

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

You said that, they're shards. But that's just djs's ShardingManager being too limited. Also, they're literally shards, and the way it instantiates shards is fixed by child_process.fork or worker_threads.spawn. Furthermore, they held a lot of Discord shard state.

Here, they're not just a single shard, but clusters of shards, they don't keep track of Discord shard state (in fact they're agnostic, this library can even be used for other libraries such as Eris, or even in stacks that have no relationship with Discord at all). And those clusters of shards are just the IDs, the classes' purpose is to define what's the way of communicating with the client, which is basically:

  • ITC (inter-thread communication, worker_threads)
  • IPC (inter-process communication, child_process.fork/cluster.fork)
  • TCP (over network, ws/http(s)/http2/quic)
  • RPC (over network, gRPC)
  • Other (Redis streams, RabbitMQ)

Similarly, those keep the same analogy and are ported to the opposite direction, so basically something like this:

flowchart LR
  A[ShardManager]
  subgraph clients
  B[ShardClient]
  C[ShardClient]
  D[ShardClient]
  end
  A <-->|Channel| B
  A <-->|Channel| C
  A <-->|Channel| D
Loading

There needs to be an opposite for the ShardManager --> ShardClient, while manager.shards may be somewhat accurate, client.shard (for ShardClient --> ShardManager) wouldn't. Furthermore, the "shard" is at the process where client lies on, it's not on client nor is client.

That's why I chose the name "channel". I understand it may be confusing for some users, but also keep in mind that sharder has no relationship with Discord, and instead, channels refer to the channel on which messages are sent.

It also makes a great analogy with real life, here we're using GitHub as a channel to communicate, but we could also be using Discord, or phones, or pigeons, or we could fly somewhere and talk face to face; the end result is always the same, the means we use to communicate is called a "communication channel", here, IChannel isn't that different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Review in Progress
Development

Successfully merging this pull request may close these issues.

2 participants