Skip to content

refactor(gateway)!: reuse queue by default in create_* functions#2116

Merged
7596ff merged 5 commits intonextfrom
7596ff/refactor/gateway/reuse-queue-in-create
Feb 5, 2023
Merged

refactor(gateway)!: reuse queue by default in create_* functions#2116
7596ff merged 5 commits intonextfrom
7596ff/refactor/gateway/reuse-queue-in-create

Conversation

@7596ff
Copy link
Contributor

@7596ff 7596ff commented Feb 4, 2023

Instead of removing these functions entirely, accept a primary Config, which is then subsequently optionally modified in the callback. The default config creates the default queue, which is then cloned and re-built before it passed to each shard. It also re-uses the TLS container.

Alternative to #2090.

Instead of removing these functions entirely, accept a primary `Config`,
which is then subsequently optionally modified in the callback. The
default config creates the default queue, which is then cloned and
re-built before it passed to each shard. It also re-uses the TLS
container.

Alternative to #2090.
@github-actions github-actions bot added c-gateway Affects the gateway crate m-breaking change Breaks the public API. t-refactor Refactors APIs or code. labels Feb 4, 2023
@vilgotf
Copy link
Member

vilgotf commented Feb 5, 2023

This is definitely an improvement but I still don't think that these functions hold their weight. As shown in #2090, they replace at most 3 LOC but are much less configurable (wich of the three functions should I use?, maybe my use case isn't covered by any one of them, etc.).

@7596ff
Copy link
Contributor Author

7596ff commented Feb 5, 2023

I think the functions serve as something we can easily point to as a bare-minimum example of how to get started. It adds a lot more range to potential users of this library. We spent a lot of time getting them right due to their importance to the rewrite. One of the requirements of the rewrite was a simple replacement for the "Cluster".

@vilgotf
Copy link
Member

vilgotf commented Feb 5, 2023

Diff of the two solutions for the stream example in twilight-gateway/README.md:

diff --git a/twilight-gateway/README.md b/twilight-gateway/README.md
index 06ce4ea578..a06b31b1d2 100644
--- a/twilight-gateway/README.md
+++ b/twilight-gateway/README.md
@@ -78,10 +75,7 @@ Create the recommended number of shards and stream over their events:
```rust,no_run
 use futures::StreamExt;
 use std::env;
-use twilight_gateway::{
-    stream::{self, ShardEventStream},
-    Config, Intents,
-};
+use twilight_gateway::{stream::ShardEventStream, Config, Intents, Shard, ShardId};
 use twilight_http::Client;
 
 #[tokio::main]
@@ -92,8 +87,9 @@ async fn main() -> anyhow::Result<()> {
      let client = Client::new(token.clone());
      let config = Config::new(token, Intents::GUILDS);

-    let mut shards = stream::create_recommended(&client, config, |_, builder| builder.build())
-        .await?
+    let recommended_shards = client.gateway().authed().await?.model().await?.shards;
+    let mut shards = (0..recommended_shards)
+        .map(|id| Shard::new(ShardId::new(id, recommended_shards), config.clone()))
         .collect::<Vec<_>>();
 
     let mut stream = ShardEventStream::new(shards.iter_mut());

@vilgotf
Copy link
Member

vilgotf commented Feb 5, 2023

It adds a lot more range to potential users of this library

One of the requirements of the rewrite was a simple replacement for the "Cluster".

I think this is the role of advertising good (starting) examples in the documentation. I don't believe users should find this or #2090 more or less confusing, but #2090 also reduces our API surface (which is nice). Additionally, I'd argue that it's easier to describe create_bucket (which users have been confused by as to why it exists) as modifying the "normal" shard creation process to (bucket_id..total).step_by(concurrency).

Copy link
Member

@Erk- Erk- left a comment

Choose a reason for hiding this comment

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

We can reuse the TlsContainer from the config parameter instead of making a new one now

@7596ff 7596ff requested a review from Erk- February 5, 2023 07:54
Copy link
Member

@vilgotf vilgotf left a comment

Choose a reason for hiding this comment

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

May also want to move some proposed changes from #2109 into this.

Comment on lines 356 to 357
/// Passing a primary config is required to ensure shared queue functionality.
/// Further customization may be performed in the callback.
Copy link
Member

@vilgotf vilgotf Feb 5, 2023

Choose a reason for hiding this comment

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

Maybe something like:

Each shard's config builder is based upon the passed config

? Mentioning the queue leaks implementation details imo.

7596ff and others added 2 commits February 5, 2023 14:20
Co-authored-by: Tim Vilgot Mikael Fredenberg <vilgot@fredenberg.xyz>
Copy link
Member

@vilgotf vilgotf left a comment

Choose a reason for hiding this comment

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

Good enough

@7596ff 7596ff merged commit 308f972 into next Feb 5, 2023
@7596ff 7596ff deleted the 7596ff/refactor/gateway/reuse-queue-in-create branch February 5, 2023 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c-gateway Affects the gateway crate m-breaking change Breaks the public API. t-refactor Refactors APIs or code.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants