Skip to content

consensus: bring back log.Error statement#4899

Merged
melekes merged 2 commits intomasterfrom
anton/3406-log-error
May 27, 2020
Merged

consensus: bring back log.Error statement#4899
melekes merged 2 commits intomasterfrom
anton/3406-log-error

Conversation

@melekes
Copy link
Contributor

@melekes melekes commented May 27, 2020

Refs #3406

cs.Logger is protected by a global mutex, so it cannot be a reason for
the halt in #3401.

@melekes
consensus: bring back log.Error statement
a590b74 
Refs #3406

cs.Logger is protected by a global mutex, so it cannot be a reason for
the halt in #3401.
@melekes melekes requested a review from tessr as a code owner May 27, 2020 08:17
@auto-comment
Copy link

auto-comment bot commented May 27, 2020 •
edited by melekes
Loading

👋 Thanks for creating a PR!

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Wrote tests
  • Updated CHANGELOG_PENDING.md
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments
  • Re-reviewed Files changed in the Github PR explorer
  • Applied Appropriate Labels

Thank you for your contribution to Tendermint! 🚀

@melekes melekes self-assigned this May 27, 2020
@melekes melekes added the R:minor Release: Minor label May 27, 2020
tac0turtle
tac0turtle approved these changes May 27, 2020
View reviewed changes
Copy link
Contributor

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

LGTM

@erikgrinaker
Copy link
Contributor

erikgrinaker commented May 27, 2020

Is the original halt problem here from a test environment? That does not surprise me, since the tests can be extremely verbose, on the order of 100 MB per run. CI environments save log output to disk, and are usually fairly resource constrained, and so if we output a lot of log data (which we do during consensus tests) then the logging may block. Not sure why this log line in particular causes it though - possibly because log.Error goes to stderr or something.

@codecov-commenter
Copy link

codecov-commenter commented May 27, 2020

Codecov Report

Merging #4899 into master will decrease coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4899      +/-   ##
==========================================
- Coverage   62.92%   62.76%   -0.16%     
==========================================
  Files         198      198              
  Lines       20011    20013       +2     
==========================================
- Hits        12592    12562      -30     
- Misses       6394     6421      +27     
- Partials     1025     1030       +5
Impacted Files Coverage Δ
consensus/state.go 73.02% <100.00%> (-1.30%) ⬇️
crypto/sr25519/pubkey.go 32.14% <0.00%> (-7.15%) ⬇️
p2p/pex/pex_reactor.go 81.56% <0.00%> (-1.68%) ⬇️
consensus/reactor.go 72.40% <0.00%> (-1.05%) ⬇️
consensus/replay.go 71.72% <0.00%> (-0.85%) ⬇️
blockchain/v0/pool.go 78.98% <0.00%> (+0.31%) ⬆️

@melekes melekes merged commit 287110d into master May 27, 2020
@melekes melekes deleted the anton/3406-log-error branch May 27, 2020 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tessr tessr Awaiting requested review from tessr

1 more reviewer

@tac0turtle tac0turtle tac0turtle approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@melekes melekes

Labels

R:minor Release: Minor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@melekes @erikgrinaker @codecov-commenter @tac0turtle