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

backport-2.1: storage: avoid spurious nodeDialer logs #31252

Merged
merged 2 commits into from Oct 11, 2018

Conversation

Projects
None yet
3 participants
@tschottdorf
Member

tschottdorf commented Oct 11, 2018

Backport 2/2 commits from #31017.

/cc @cockroachdb/release


breaker.Success() retrieves its count from an underlying windowed
counter that resets every 10 seconds. As a result, spurious "established
connection to nX" messages would pop up every 10s.

It turns out it's annoyingly difficult to properly gauge whether an
established connection is the first of its kind. In trying to do so, I
became unconvinced that this is even worth it, so I ended up removing
it. I similarly simplified the logging in the unsuccessful case (though
ConsecutiveFailures is actually not a windowed counter and presumably
worked correctly).

I also downgraded the message in the error case to Infof, as it's
expected to see it when nodes are restarted (which is a routine
operation and nothing to worry about). We want to keep the Warning level
for true warnings.

Release note: None

@tschottdorf tschottdorf requested a review from cockroachdb/core-prs as a code owner Oct 11, 2018

@cockroach-teamcity

This comment has been minimized.

Show comment
Hide comment
@cockroach-teamcity

cockroach-teamcity Oct 11, 2018

Member

This change is Reviewable

Member

cockroach-teamcity commented Oct 11, 2018

This change is Reviewable

@petermattis

This comment has been minimized.

Show comment
Hide comment
@petermattis

petermattis Oct 11, 2018

Contributor
Contributor

petermattis commented Oct 11, 2018

tschottdorf added some commits Oct 5, 2018

storage: avoid spurious nodeDialer logs
breaker.Success() retrieves its count from an underlying windowed
counter that resets every 10 seconds. As a result, spurious "established
connection to nX" messages would pop up every 10s.

It turns out it's annoyingly difficult to properly gauge whether an
established connection is the first of its kind. In trying to do so, I
became unconvinced that this is even worth it, so I ended up removing
it. I similarly simplified the logging in the unsuccessful case (though
ConsecutiveFailures is actually not a windowed counter and presumably
worked correctly).

I also downgraded the message in the error case to Infof, as it's
expected to see it when nodes are restarted (which is a routine
operation and nothing to worry about). We want to keep the Warning level
for true warnings.

Release note: None
nodedialer: Reset() breaker only when it is tripped
This is slightly better since it only resets the breaker state (and with
it the underlying counters) when it's tripped.

This shouldn't result in any external changes but made the problem fixed
in #31017 much more visible than it would've otherwise been.

Release note: None

@tschottdorf tschottdorf merged commit 062109d into cockroachdb:release-2.1 Oct 11, 2018

2 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
license/cla Contributor License Agreement is signed.
Details

@tschottdorf tschottdorf deleted the tschottdorf:backport2.1-31017 branch Oct 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment