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

fix(cluster) quiet keepalive events #1660

Merged
merged 2 commits into from
Sep 20, 2016
Merged

fix(cluster) quiet keepalive events #1660

merged 2 commits into from
Sep 20, 2016

Conversation

subnetmarco
Copy link
Member

Full changelog

  • Stops propagating cluster keep-alive requests, reducing intracluster noise

-- @treturn table res A table representing the insert row (with fields created during the insertion).
-- @treturn table err If an error occured, a table describing the issue.
function DAO:insert(tbl, options)
check_arg(tbl, 1, "table")
options = options or {}
Copy link
Member

Choose a reason for hiding this comment

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

Should be:

function DAO:insert(tbl, options)
  options = options or {}
  check_arg(tbl, 1, "table")            
  check_arg(options, 2, "table")

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -82,7 +82,7 @@ local function send_keepalive(premature)
ngx_log(ngx.ERR, tostring(err))
elseif #nodes == 1 then
local node = nodes[1]
local _, err = singletons.dao.nodes:update(node, node, {ttl=singletons.configuration.cluster_ttl_on_failure})
local _, err = singletons.dao.nodes:update(node, node, {ttl=singletons.configuration.cluster_ttl_on_failure, quiet = true})
Copy link
Member

Choose a reason for hiding this comment

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

Could we avoid long lines? As per our codestyle, not exceeding 80 chars width is ideal:

local _, err = singletons.dao.nodes:update(node, node, {
  ttl = singletons.configuration.cluster_ttl_on_failure, 
  quiet = true
})

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@thibaultcha thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/needs review labels Sep 20, 2016
@thibaultcha thibaultcha added pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Sep 20, 2016
@Tieske
Copy link
Member

Tieske commented Sep 20, 2016

two questions;

  1. no tests?
  2. shouldn't, for consistency, the quiet option be implemented for DELETE as well?

@thibaultcha
Copy link
Member

no tests?

Not testable, at least none are written regarding events propagation (other than invalidation), and as we know they are extremely unreliable. Testing that an event was not propagated is even trickier.

shouldn't, for consistency, the quiet option be implemented for DELETE as well?

Eventually. Before the end of the day if so, because 0.9.2 is to be released tomorrow. Otherwise will merge this as-is.

@thibaultcha
Copy link
Member

Has been implemented in delete as well.

@thibaultcha thibaultcha merged commit 272d956 into next Sep 20, 2016
@thibaultcha thibaultcha deleted the fix/cluster branch September 20, 2016 21:21
@subnetmarco
Copy link
Member Author

I was working on the tests

thibaultcha pushed a commit that referenced this pull request Sep 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants