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 package #7204

Closed
wants to merge 1 commit into from

Conversation

kyranet
Copy link
Member

@kyranet kyranet commented Jan 8, 2022

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

I heard you wanted a nice and complete sharder. 👀

This sharder draws inspiration from the strategy and plug-in system Sapphire has largely adopted over the months, the way Kurasuta handles messages and shards separately, and the structure of Discord.js's built-in sharding manager to make the update easy, alongside a few sprinkles of my own based on my experience when building a cross-process intercommunication system.

⚠️ This PR is under heavy WIP: docs and tests are missing, they'll be done as I progress. The API may also change as new needs arise when building more code.

Let's talk about this sharder's structure! It is divided in three components:

  • The Sharding Manager: it's the central class that handles the shards, from starting them up, to closing.
  • The Shards: they're strategies that define how the "channel" should be handled from the sharding manager, they can be of any kind, from workers (WorkerShardHandler) and forks (ForkProcessShardHandler) to cross-server clusters going thru local clusters (ClusterProcessShardHandler). Yes, you heard that. Although we'll most likely ship only the 3 local ones. You can make your own custom shard strategies.
  • The Messengers: they're also strategies, but they define how data must be serialized and/or deserialized as well as handling message queues, we'll provide RawMessageHandler (data as-is, without converting to a Buffer or a string), JsonMessageHandler (JSON), and BinaryMessageHandler (V8 serder)... You can plug in your own ones, such as the Erlang External Term Format (ETF), YAML, TOML, etc.

This clear separation between responsibilities allow for greater customization of the core functionality of the library, reducing the need to use external libraries.

And coming in the future, we'll probably release The Message Mappers, which give a final touch-up to a message, such as appending a header/footer or even, using OpenSSL to encrypt messages over the network.

Issues to handle:

Todo:

  • Consolidate the sharder's API, make everything more consistent.
  • Make ProcessShardHandler abstract, for the two classes below.
  • Implement ForkProcessShardHandler.
  • Implement CusterProcessShardHandler.
  • Implement WorkerShardHandler.
  • Implement RawMessageHandler.
  • Implement BinaryMessageHandler.
  • Implement built-in cluster sharding (e.g. 2 processes with 5 shards each to have 10 shards) support.
  • Implement fetchRecommendedShards.
  • Implement shard client to interact with the shard manager, ShardTunnel? ShardClient?
  • Type the events.
  • Add documentation.
  • Add tests.

Note: This is a port of discordjs/discord.js-modules#87

Status and versioning classification:

  • I know how to update typings and have done so, or typings don't need updating
  • This PR changes the library's interface (methods or parameters added)

@xhyrom
Copy link
Contributor

xhyrom commented Jan 8, 2022

🎉

@meister03
Copy link
Contributor

Is there a reason, why @discordjs/sharder module is using the cluster module instead of child-processes. The Cluster module has some layers built on top of child_processes, which can make it slower.
Or has it been chosen, bc it allows using the same port?

@kyranet
Copy link
Member Author

kyranet commented Jan 9, 2022

I fail to see where you see cluster being the only/forced option to use, @meister03, it's simply a strategy.

If somebody wants to use cluster over child processes, they're free to do so.

@Deivu
Copy link
Contributor

Deivu commented Jan 30, 2022

This looks promising as a replacement for old d.js sharder. One thing though, can we implement our op system in this sharder by tapping on ipc messages, and do the same send() reply() fashion like how the old veza does? eg: if we want to send messages our selves from main process to sub process? One reason why I would like a system like this, is because I don't want to use broadcastEval on getting data across my sub process.

@meister03
Copy link
Contributor

This looks promising as a replacement for old d.js sharder. One thing though, can we implement our op system in this sharder by tapping on ipc messages, and do the same send() reply() fashion like how the old veza does? eg: if we want to send messages our selves from main process to sub process? One reason why I would like a system like this, is because I don't want to use broadcastEval on getting data across my sub process.

You can reply to messages, thats been done with the track and waitforId functions(MessageHandler), when I recognized right.

@DraftProducts
Copy link
Contributor

DraftProducts commented Feb 9, 2022

I think there are a few features from Discordeno that we could learn from and would be a big improvement for discord.js and big bots that use it.

Actually, updates are painful for bots that handle a large amount of shards.

https://github.com/discordeno/discordeno#gateway
image

@vladfrangu
Copy link
Member

A lot of those would go in the @discordjs/ws package, not in sharder, and are actually planned to a degree (for instance you can host the ws in a different process or machine, and in the future, you can make d.js pull from that process using NATS or whatever floats your boat)!
I'd also like to clarify that technically there is "downtime" when resharding, as the connections have to close and reopen with an identify payload being sent with the new shard configuration but 🤫

@iCrawl
Copy link
Member

iCrawl commented Feb 9, 2022

I would love to take inspiration of discordeno, if it were any good that is.

Most of that text is just random fluff with no actual improvement or impact, or a complete lack and misunderstanding of how sharding works to make it look more appealing.

@DraftProducts
Copy link
Contributor

DraftProducts commented Feb 9, 2022

Sure @iCrawl, I agree with you.
Sadly, like any promotional text, it can happen that the reality is embellished (#LinkedIn), but behind these beautiful words, there is necessarily something concrete and/or a concept that could improve Discord.js.
I'm sure that downtime can be optimized.
If we use the current ShardingManager spawn system with a large amount of shards, an update of is painful.
I am convinced that we can find a way to launch the shards in a more optimized way, if only with the max_concurrency implemented by Discord :
The way that I currently use, is a custom class in the ShardingManager that communicate through an external Pub-sub with each Shard to resolve a Promise that block the IDENTIFY request of WebSocketShard class to start each shard with the minimal downtime without a rate limit (thx to @Koyamie for his help for that stuff).

@discordjs discordjs deleted a comment from burnthoney Feb 13, 2022
@Drxckzyz
Copy link

A lot of those would go in the @discordjs/ws package, not in sharder, and are actually planned to a degree (for instance you can host the ws in a different process or machine, and in the future, you can make d.js pull from that process using NATS or whatever floats your boat)!
I'd also like to clarify that technically there is "downtime" when resharding, as the connections have to close and reopen with an identify payload being sent with the new shard configuration but 🤫

Actually it is close to zero downtime as you only lose a few events, you have the new shard and the old shards stays connected until the new one is ready

@vercel
Copy link

vercel bot commented Jul 15, 2022

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

1 Ignored Deployment
Name Status Preview Updated
discord-js ⬜️ Ignored (Inspect) Jul 15, 2022 at 9:33PM (UTC)

@phamleduy04
Copy link

any updates on this?

@JPBM135 JPBM135 mentioned this pull request Sep 30, 2022
@kyranet kyranet mentioned this pull request Nov 24, 2022
35 tasks
@kyranet
Copy link
Member Author

kyranet commented Nov 24, 2022

Superseded by #8859, the new PR is up-to-date and implements the RFC. Also has a much simpler API.

@kyranet kyranet closed this Nov 24, 2022
@kyranet kyranet deleted the feat/added-sharder-package branch November 24, 2022 15:26
@almeidx almeidx removed the blocked label Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants