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

fix(console sink): Upgrade console sink for tokio 0.2 #2096

Merged
merged 4 commits into from
Mar 19, 2020

Conversation

MOZGIII
Copy link
Contributor

@MOZGIII MOZGIII commented Mar 19, 2020

Signed-off-by: MOZGIII mike-n@narod.ru

@github-actions
Copy link

Great PR! Please pay attention to the following items before merging:

Files matching src/**:

  • For each failure path, is there sufficient context logged for users to investigate the issue?
  • Do the tests ensure that behavior is sane for inputs that don't meet normal assumptions (e.g. missing field, non-string, etc)?
  • Did you add adequate documentation?

This is an automatically generated QA checklist based on modified files

Copy link
Contributor

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

:shipit: 🔥

src/sinks/console.rs Outdated Show resolved Hide resolved
Copy link
Member

@lukesteensen lukesteensen left a comment

Choose a reason for hiding this comment

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

FramedWrite gave us some buffering on the output. Did we lose that here? Might be worth checking if throughput changed at all.

Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII MOZGIII merged commit 07343ac into master Mar 19, 2020
@MOZGIII
Copy link
Contributor Author

MOZGIII commented Mar 19, 2020

FramedWrite gave us some buffering on the output. Did we lose that here? Might be worth checking if throughput changed at all.

This is a good idea, I'll create an issue. UPD: #2099
Merging this because it's a hotfix, and we're going to apply fixes later.

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.

3 participants