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

Optimize jetstream::consumer::pull::Consumer::stream method. #529

Merged
merged 15 commits into from
Jun 27, 2022

Conversation

Jarema
Copy link
Member

@Jarema Jarema commented Jun 23, 2022

No description provided.

@Jarema Jarema requested a review from caspervonb June 23, 2022 08:13
@Jarema Jarema force-pushed the jarema/pull-stream-rework branch from d69ef8e to 5e3eb1b Compare June 23, 2022 08:23
Copy link
Collaborator

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

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

Two nits

async-nats/src/jetstream/consumer/pull.rs Show resolved Hide resolved
async-nats/src/jetstream/consumer/pull.rs Show resolved Hide resolved
async-nats/src/jetstream/consumer/pull.rs Outdated Show resolved Hide resolved
async-nats/src/jetstream/consumer/pull.rs Outdated Show resolved Hide resolved
async-nats/src/jetstream/consumer/pull.rs Outdated Show resolved Hide resolved
@Jarema Jarema force-pushed the jarema/pull-stream-rework branch from ece547f to 0f9f588 Compare June 24, 2022 08:42
Copy link
Collaborator

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

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

Alright so punting the problems we ran into?

Last bit I'm curious about is the relative performance to main.
Something simple like counting messages over a time span would be a good indicator here.

@Jarema
Copy link
Member Author

Jarema commented Jun 27, 2022

new approach 735 ms.
sequence: 10200 ms.

Asserted payloads etc.

)
.await
}
pub async fn stream_with_config(&self, config: BatchConfig) -> Result<Stream<'_>, Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we know we can easily run into issues with this, maybe we wanna hold off on the with_config variant til after this week's release?

Suggested change
pub async fn stream_with_config(&self, config: BatchConfig) -> Result<Stream<'_>, Error> {
pub async fn stream_with_config(&self, config: BatchConfig) -> Result<Stream<'_>, Error> {

Copy link
Member Author

@Jarema Jarema Jun 27, 2022

Choose a reason for hiding this comment

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

@caspervonb I will follow up today with:

  1. make this private
  2. expose only batch, max bytes and heartbeats.

It's not released yet, so I would not bother with removing it, as it requires removing tests that will be needed the same day (pull with heartbeat).

Copy link
Collaborator

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

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

lgtm (remember to squash 😁)

@Jarema Jarema merged commit aaf626b into main Jun 27, 2022
@Jarema Jarema deleted the jarema/pull-stream-rework branch June 27, 2022 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants