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

reduce WARN logging to only necessary scenario #33408

Merged
merged 1 commit into from
Sep 28, 2023

Conversation

tao-stones
Copy link
Contributor

Problem

A WARN log being excessively triggered by vote-only slots. It was meant to log an unlikely event when duplicated bank had prioritized TXs but the confirmed bank had none. It is not entirely impossible, but maybe something worth looking into, hence logging.

Summary of Changes

  • add comments and limits to reduce the logging to targeted scenario

Fixes #

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #33408 (6ec68aa) into master (d25d53e) will increase coverage by 0.0%.
The diff coverage is 50.0%.

@@           Coverage Diff           @@
##           master   #33408   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         798      798           
  Lines      216974   216974           
=======================================
+ Hits       177902   177905    +3     
+ Misses      39072    39069    -3     

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

This looks okay to me, but I don't have the complete context from the original changeset. It might be a good idea to have one of your reviewers from that PR approve this.

Please backport to v1.16 to prevent spamming the (soon to be) stable TestValidator with these warnings.

@tao-stones tao-stones requested a review from ryoqun September 26, 2023 14:09
@tao-stones tao-stones added v1.14 v1.16 PRs that should be backported to v1.16 labels Sep 26, 2023
@tao-stones tao-stones requested a review from apfitzge September 26, 2023 14:11
Copy link
Contributor

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

lgtm

@tao-stones tao-stones merged commit cc4e928 into solana-labs:master Sep 28, 2023
1 check passed
mergify bot pushed a commit that referenced this pull request Sep 28, 2023
mergify bot pushed a commit that referenced this pull request Sep 28, 2023
tao-stones added a commit that referenced this pull request Sep 28, 2023
…3408) (#33444)

reduce WARN logging to only necessary scenario (#33408)

(cherry picked from commit cc4e928)

Co-authored-by: Tao Zhu <[email protected]>
tao-stones added a commit that referenced this pull request Sep 28, 2023
…3408) (#33445)

reduce WARN logging to only necessary scenario (#33408)

(cherry picked from commit cc4e928)

Co-authored-by: Tao Zhu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.16 PRs that should be backported to v1.16
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants