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

Store event counters separately for each privacyIDEA node #1833

Merged
merged 5 commits into from Sep 6, 2019

Conversation

@fredreichbier
Copy link
Member

commented Sep 3, 2019

This PR implements one half of #1819: It modifies the eventcounter table such that each privacyIDEA node writes to its own "private" row. For retrieving the value of a given counter, the counter values of all rows of this counter are summed up.

This hopefully reduces the number of deadlocks in a replicated setup that occur because multiple nodes simultaneously try to write to the same table row. This does actually work, see #1819.

Writing to the correct row is completely handled by the lib level, so the API level does not need to be changed.

This also modifies the increase/decrease API a bit: We now don't return the new counter value -- this would require an additional SQL query, and the result is actually only used to write to the debug log.

Writing a migration script turned out to be a bit complicated because counter_name was the primary key before, and we now need an primary id key. But alembic apparently cannot add a new primary key to an existing table, so we have to create a new table, migrate the data, drop the old table and rename the old one.

Event counters: Reduce number of SQL queries
This avoids several SQL queries for irrelevant data.

@fredreichbier fredreichbier changed the title Store event counters separately for privacyIDEA node Store event counters separately for each privacyIDEA node Sep 3, 2019

@fredreichbier fredreichbier requested a review from cornelinux Sep 5, 2019

@fredreichbier fredreichbier marked this pull request as ready for review Sep 5, 2019

@fredreichbier fredreichbier added this to In progress in privacyIDEA 3.2 via automation Sep 5, 2019

return db.session.query(func.sum(EventCounter.counter_value))\
.filter(EventCounter.counter_name == counter_name).one()[0]

This comment has been minimized.

Copy link
@cornelinux

cornelinux Sep 6, 2019

Member

Thats a lot of logic in one line! ;-)

log.debug(u"Increased the counter {0!s} to {1!s}.".format(counter_name,
r))
increase(counter_name)
log.debug(u"Increased the counter {0!s}.".format(counter_name))

This comment has been minimized.

Copy link
@cornelinux

cornelinux Sep 6, 2019

Member

We could have also returned the overall new counter value (over all nodes). This would have kept the log entry the same. But it is not that important to me.

This comment has been minimized.

Copy link
@fredreichbier

fredreichbier Sep 6, 2019

Author Member

Yes, I agree that would be nice. But this results in another SELECT query -- which, I think, would be totally fine if the result is used for something important. But as it was only written to the debug log, I thought we could save one query and speed up things a bit :)

def increase_and_read(name):
""" helper function that increases the event counter and returns the new value """
increase(name)
return read(name)

This comment has been minimized.

Copy link
@cornelinux

cornelinux Sep 6, 2019

Member

Why wouldn't we do this in increase or decrease? Performance and avoiding deadlocks?

This comment has been minimized.

Copy link
@fredreichbier

fredreichbier Sep 6, 2019

Author Member

For performance, mostly (see above) :) Also I didn't want to change all occurrences of
r = increase(...) in the tests to

increase(...)
r = read(...)

so I decided to go for the helper method.

@cornelinux

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

@fredreichbier I had some minor comments. But none of them which keep me from merging this. Maybe you get some a 2nd thought or me some hint :-)

privacyIDEA 3.2 automation moved this from In progress to Reviewer approved Sep 6, 2019

@cornelinux cornelinux merged commit cdcf528 into master Sep 6, 2019

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 97.08%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +2.91% compared to dc6f2bb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

privacyIDEA 3.2 automation moved this from Reviewer approved to Done Sep 6, 2019

@cornelinux cornelinux deleted the 1819/counter-per-node branch Sep 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.