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.

Sign up for GitHub

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

Jump to bottom

[improve][broker] ServerCnx: log at warning level when topic not found #16225

Merged
merged 1 commit into from
Oct 12, 2022
Merged

[improve][broker] ServerCnx: log at warning level when topic not found #16225

merged 1 commit into from
Oct 12, 2022

Conversation

michaeljmarshall
Copy link
Member

@michaeljmarshall michaeljmarshall commented Jun 26, 2022 •
edited by github-actions bot
Loading

Motivation

In #13950, we switched an error log to an info log. @hsaputra requested on Pulsar Slack we use warn level so that alerts still fire in for the case when the topic is not found.

Modifications

  • Replace info with warn when topic is not found in the ServerCnx class.
  • doc-not-needed

@michaeljmarshall michaeljmarshall added type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use area/broker doc-not-needed Your PR changes do not impact docs release/2.10.2 labels Jun 26, 2022
@michaeljmarshall michaeljmarshall added this to the 2.11.0 milestone Jun 26, 2022
@michaeljmarshall michaeljmarshall self-assigned this Jun 26, 2022
@hsaputra
Copy link

LGTM +1(non binding)

merlimat
merlimat reviewed Jun 27, 2022
View reviewed changes
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

I'd say info is more appropriate here. There is nothing wrong on the broker, just a client is trying to use a topic that does not exist (and auto-creation is turned off).

Also, WARN typically will not trigger an alert (as they would be super-noisy with lot of false positives).

@hsaputra
Copy link

@merlimat - this indicates something is wrong and need to be fixed in either client or server.
It is "noise" that needs attention based on out experience running in production bc indeed something not matching.

@dave2wave
Copy link
Member

When I'm looking at broker logs I am scanning for WARN and ERROR. While this is an user error it is still something the operator may need to help their user identify.

@dave2wave
Copy link
Member

@merlimat on the clusters that you operate are you paged on WARN? If not then I don't see this change as noise.

@codelipenghui codelipenghui modified the milestones: 2.11.0, 2.12.0 Jul 26, 2022
@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Aug 25, 2022
Jason918
Jason918 approved these changes Aug 27, 2022
View reviewed changes
Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

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

LGTM

@Jason918
Copy link
Contributor

@merlimat PTAL

@github-actions github-actions bot removed the Stale label Aug 28, 2022
@github-actions
Copy link

github-actions bot commented Oct 5, 2022

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Oct 5, 2022
@michaeljmarshall
Copy link
Member Author

Triggering a new CI build.

@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2022 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@ce24db1). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #16225   +/-   ##
=========================================
  Coverage          ?   51.05%           
  Complexity        ?     8724           
=========================================
  Files             ?      607           
  Lines             ?    53397           
  Branches          ?     5712           
=========================================
  Hits              ?    27260           
  Misses            ?    23072           
  Partials          ?     3065
Flag Coverage Δ
unittests 51.05% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@michaeljmarshall michaeljmarshall changed the title [broker] ServerCnx: log at warning level when topic not found [improve][broker] ServerCnx: log at warning level when topic not found Oct 11, 2022
@michaeljmarshall michaeljmarshall added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages and removed type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use labels Oct 11, 2022
@github-actions github-actions bot removed the Stale label Oct 12, 2022
@michaeljmarshall michaeljmarshall merged commit 72e0445 into apache:master Oct 12, 2022
@michaeljmarshall michaeljmarshall deleted the use-warn-for-error branch October 12, 2022 05:06
michaeljmarshall added a commit that referenced this pull request Oct 12, 2022
@michaeljmarshall
[broker] ServerCnx: log at warning level when topic not found (#16225)
58dfefc 
### Motivation

In #13950, we switched an error log to an info log. @hsaputra requested on Pulsar Slack we use warn level so that alerts still fire in for the case when the topic is not found.

### Modifications

* Replace `info` with `warn` when topic is not found in the `ServerCnx` class.
- [x] `doc-not-needed`

(cherry picked from commit 72e0445)
michaeljmarshall added a commit that referenced this pull request Oct 12, 2022
@michaeljmarshall
[broker] ServerCnx: log at warning level when topic not found (#16225)
429a81c 
### Motivation

In #13950, we switched an error log to an info log. @hsaputra requested on Pulsar Slack we use warn level so that alerts still fire in for the case when the topic is not found.

### Modifications

* Replace `info` with `warn` when topic is not found in the `ServerCnx` class.
- [x] `doc-not-needed`

(cherry picked from commit 72e0445)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Oct 12, 2022
@michaeljmarshall @nicoloboschi
[broker] ServerCnx: log at warning level when topic not found (apache…
15c1fe9 
…#16225)

### Motivation

In apache#13950, we switched an error log to an info log. @hsaputra requested on Pulsar Slack we use warn level so that alerts still fire in for the case when the topic is not found.

### Modifications

* Replace `info` with `warn` when topic is not found in the `ServerCnx` class.
- [x] `doc-not-needed`

(cherry picked from commit 72e0445)
(cherry picked from commit 58dfefc)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merlimat merlimat merlimat approved these changes

@lhotari lhotari lhotari approved these changes

@Jason918 Jason918 Jason918 approved these changes

@hangc0276 hangc0276 Awaiting requested review from hangc0276

@eolivelli eolivelli Awaiting requested review from eolivelli

@315157973 315157973 Awaiting requested review from 315157973

@nicoloboschi nicoloboschi Awaiting requested review from nicoloboschi

@mattisonchao mattisonchao Awaiting requested review from mattisonchao

Assignees

@michaeljmarshall michaeljmarshall

Labels
area/broker cherry-picked/branch-2.10 cherry-picked/branch-2.11 doc-not-needed Your PR changes do not impact docs release/2.10.3 release/2.11.1 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
8 participants
@michaeljmarshall @hsaputra @dave2wave @Jason918 @codecov-commenter @merlimat @lhotari @codelipenghui