Skip to content

Slave lag throttler symbolize config keys#55

Merged
bbuchalter merged 2 commits into
masterfrom
update-slave-connection-log-statement
Sep 4, 2018
Merged

Slave lag throttler symbolize config keys#55
bbuchalter merged 2 commits into
masterfrom
update-slave-connection-log-statement

Conversation

@nataliebettenburg

Copy link
Copy Markdown

The Billing team was trying to use the Slave Lag Throttler to run some LHMs and it wasn't behaving as expected. While investigating, we noticed a log statement like:
"Connecting to 35.***.***.31 on database: "
which made us think that the config was missing the database entry. Turns out it was just the log statement that was incorrect/misleading (config['database'] vs config[:database]).

I am proposing to fix this by symbolizing the keys in the config and using symbols to access the values.

I added a test and also refactored a couple of the existing tests since the assertions in those tests were not being executed.

@nataliebettenburg

Copy link
Copy Markdown
Author

@bbuchalter

cc @melari @kevinodotnet

Also, don't use a method definition where a variable will do.
@bbuchalter

Copy link
Copy Markdown

Thanks for your contribution here! ❤️

I've tacked on a bit of polish as well. I'm happy with it, but now that I'm involved, I'm going to get an extra set of eyes on it.

@bbuchalter bbuchalter requested a review from insom September 4, 2018 20:18

@insom insom left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for your contributions!

log_messages = @logs.string.lines
assert_equal(2, log_messages.length)
assert log_messages[0].include? "Connecting to slave on database: db"
assert log_messages[1].include? "Error connecting to slave: Unknown MySQL server host 'slave'"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

@bbuchalter bbuchalter merged commit c224f3d into master Sep 4, 2018
@bbuchalter bbuchalter deleted the update-slave-connection-log-statement branch September 4, 2018 20:33
@bbuchalter

Copy link
Copy Markdown

Will cut a new version of this gem shortly with this PR.

@bbuchalter

Copy link
Copy Markdown

A new release has been cut. Thanks again! ❤️

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants