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

Properly cache zookeeper connection strings #3333

Merged
merged 5 commits into from
Mar 21, 2019
Merged

Conversation

nmuesch
Copy link
Collaborator

@nmuesch nmuesch commented Mar 20, 2019

What does this PR do?

Properly uses the last_zk seen cache for the case when users provided a list of connection strings. Before we were assuming there was only one.

Also adds the exception to an error message we provide.

Motivation

We accept both a string and a list of strings for zk_connect_str in the yaml file. However, the _should_zk function was attempting to use this to access a dictionary. In the case of it being a list, you'd end up with an exception about list being an unhashable type. This addresses that, with a test :)

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • Feature or bugfix must have tests
  • Git history must be clean
  • If PR adds a configuration option, it must be added to the configuration file.

@codecov
Copy link

codecov bot commented Mar 20, 2019

Codecov Report

Merging #3333 into master will decrease coverage by 5.37%.
The diff coverage is 87.5%.

@@            Coverage Diff            @@
##           master   #3333      +/-   ##
=========================================
- Coverage   85.97%   80.6%   -5.38%     
=========================================
  Files         700       8     -692     
  Lines       36908     531   -36377     
  Branches     4453     113    -4340     
=========================================
- Hits        31731     428   -31303     
+ Misses       3956      72    -3884     
+ Partials     1221      31    -1190

@nmuesch nmuesch force-pushed the nick/fix_consumer branch from bd0bbb1 to 8e4e486 Compare March 21, 2019 09:34
@@ -97,3 +99,15 @@ def test_check_nogroups_zk(aggregator, zk_instance):
else:
for mname in BROKER_METRICS + CONSUMER_METRICS:
aggregator.assert_metric(mname, at_least=1)


def test_should_zk():
Copy link
Contributor

Choose a reason for hiding this comment

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

💜

@nmuesch nmuesch merged commit 6833fc6 into master Mar 21, 2019
@nmuesch nmuesch deleted the nick/fix_consumer branch March 21, 2019 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants