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.

Sign up for GitHub

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

Jump to bottom

Non-blocking Redis calls: preparation (import libs, write async client, adapt tests, etc.) #77

Merged
merged 26 commits into from
Mar 4, 2019
Merged

Non-blocking Redis calls: preparation (import libs, write async client, adapt tests, etc.) #77

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
b66918d
gemspec: set Ruby to >= 2.3.0
davidor Dec 5, 2018
d0cda35
ruby-version: set to 2.3.6
davidor Dec 5, 2018
b988de1
.circleci/config: do not run tests on ruby 2.2
davidor Feb 25, 2019
babe251
Gemfiles: add async-redis
davidor Dec 5, 2018
15c9d32
test/test_helper: run tests with a reactor
davidor Feb 4, 2019
f687302
spec/acceptance/utilization: delete unnecessary 'before'
davidor Feb 4, 2019
b4dd9a5
spec/spec_helper: run tests with a reactor
davidor Feb 4, 2019
2e7ee3e
configuration: add redis.async param
davidor Feb 22, 2019
5e09973
Add an async client based on async-redis
davidor Feb 4, 2019
0dbb2d5
StorageAsync: implement pipelining
davidor Feb 19, 2019
60850db
StorageAsync: add .call_pipeline method to the async-redis lib
davidor Feb 19, 2019
1e0571c
test/helpers/storage: use StorageAsync::Client instead of Storage
davidor Feb 14, 2019
66c30da
spec/unit: add specs for Pipeline
davidor Feb 25, 2019
b636e86
storage: create StorageSync for the redis-rb based storage
davidor Feb 22, 2019
9970253
storage: add new class that chooses async or sync based on the config
davidor Feb 22, 2019
215f986
test/test_helpers/configuration: set redis.async = true
davidor Feb 22, 2019
e065c65
test/unit: adapt storage tests
davidor Feb 25, 2019
c65546d
spec/unit/event_storage: run test with threads only when using sync s…
davidor Feb 25, 2019
d9308a6
oauth/token_storage: do not use .lazy for the smembers call
davidor Feb 25, 2019
8572f5e
Dockerfile.ci: do not install Ruby 2.2
davidor Feb 26, 2019
1d90d27
storage_async/client: initialize properly using params in configurati…
davidor Feb 26, 2019
a59aacd
Gemfiles: update async-redis to v0.3.3
davidor Mar 1, 2019
39cf772
storage: expose #new
davidor Mar 4, 2019
0cef4d6
queue_storage, stats_storage: instantiate Storage with Storage#new
davidor Mar 4, 2019
9035dbd
spec/unit/queue_storage: run tests only when using sync storage
davidor Mar 4, 2019
ba5a046
storage_async/pipeline: check that it is not shared between different…
davidor Mar 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,6 @@ jobs:
key: v1-dependencies-{{ checksum "Gemfile.lock" }}

#run tests!
- run:
name: Run tests on Ruby 2.2
command: |
TEST_RUBY_VERSION=2.2 script/ci

- run:
name: Run tests on Ruby 2.3
command: |
Expand Down
2 changes: 1 addition & 1 deletion .ruby-version
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
2.2.4
2.3.6
2 changes: 1 addition & 1 deletion 3scale_backend.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ Gem::Specification.new do |s|
s.description = 'This gem provides a daemon that handles authorization and reporting of web services managed by 3scale.'
s.license = 'Apache-2.0'

s.required_ruby_version = ">= 2.2.0"
s.required_ruby_version = ">= 2.3.0"
s.required_rubygems_version = ">= 1.3.7"

s.files = Dir.glob('{lib,bin,app,config}/**/*')
Expand Down
2 changes: 1 addition & 1 deletion Dockerfile.ci
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ RUN sudo chown -R "${USER_NAME}": /tmp/apisonator \
&& rbenv_update_env

# specify all versions to be installed, partial versions also understood
ARG RUBY_VERSIONS="2.2 2.3"
ARG RUBY_VERSIONS="2.3"
RUN cd /tmp/apisonator \
&& ruby_versions ${RUBY_VERSIONS}

Expand Down
2 changes: 2 additions & 0 deletions Gemfile.base
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ group :test do
gem 'rspec', '~> 3.7.0', require: nil
gem 'geminabox', '~> 0.13.11', require: false
gem 'codeclimate-test-reporter', '~> 0.6.0', require: nil
gem 'async-rspec'
end

group :development do
Expand Down Expand Up @@ -60,3 +61,4 @@ gem 'sinatra', '~> 2.0.3'
gem 'sinatra-contrib', '~> 2.0.3'
# Optional external error logging services
gem 'bugsnag', '~> 6', require: nil
gem 'async-redis', '~> 0.3.3', require: nil
14 changes: 14 additions & 0 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,16 @@ GEM
airbrake (4.3.1)
builder
multi_json
async (1.15.2)
nio4r (~> 2.3)
timers (~> 4.1)
async-io (1.18.5)
async (~> 1.14)
async-redis (0.3.3)
async (~> 1.8)
async-io (~> 1.10)
async-rspec (1.12.0)
rspec (~> 3.0)
aws-sdk (2.4.2)
aws-sdk-resources (= 2.4.2)
aws-sdk-core (2.4.2)
Expand Down Expand Up @@ -110,6 +120,7 @@ GEM
net-scp (1.2.1)
net-ssh (>= 2.6.5)
net-ssh (4.2.0)
nio4r (2.3.1)
nokogiri (1.9.1)
mini_portile2 (~> 2.4.0)
pg (0.20.0)
Expand Down Expand Up @@ -188,6 +199,7 @@ GEM
thread_safe (0.3.6)
tilt (2.0.8)
timecop (0.9.1)
timers (4.3.0)
tzinfo (1.2.4)
thread_safe (~> 0.1)
vegas (0.1.11)
Expand All @@ -205,6 +217,8 @@ PLATFORMS
DEPENDENCIES
3scale_backend!
airbrake (= 4.3.1)
async-redis (~> 0.3.3)
async-rspec
aws-sdk (= 2.4.2)
benchmark-ips (~> 2.7.2)
bugsnag (~> 6)
Expand Down
14 changes: 14 additions & 0 deletions Gemfile.on_prem.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,16 @@ GEM
i18n (~> 0.7)
minitest (~> 5.1)
tzinfo (~> 1.1)
async (1.15.2)
nio4r (~> 2.3)
timers (~> 4.1)
async-io (1.18.5)
async (~> 1.14)
async-redis (0.3.3)
async (~> 1.8)
async-io (~> 1.10)
async-rspec (1.12.0)
rspec (~> 3.0)
backports (3.11.3)
benchmark-ips (2.7.2)
bugsnag (6.6.4)
Expand Down Expand Up @@ -99,6 +109,7 @@ GEM
net-scp (1.2.1)
net-ssh (>= 2.6.5)
net-ssh (4.2.0)
nio4r (2.3.1)
nokogiri (1.9.1)
mini_portile2 (~> 2.4.0)
pkg-config (1.1.9)
Expand Down Expand Up @@ -174,6 +185,7 @@ GEM
thread_safe (0.3.6)
tilt (2.0.8)
timecop (0.9.1)
timers (4.3.0)
tzinfo (1.2.4)
thread_safe (~> 0.1)
vegas (0.1.11)
Expand All @@ -188,6 +200,8 @@ PLATFORMS

DEPENDENCIES
3scale_backend!
async-redis (~> 0.3.3)
async-rspec
benchmark-ips (~> 2.7.2)
bugsnag (~> 6)
builder (= 3.2.3)
Expand Down
3 changes: 2 additions & 1 deletion lib/3scale/backend/configuration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ def parse_int(value, default)
config.add_section(:queues, :master_name, :sentinels, :role,
:connect_timeout, :read_timeout, :write_timeout)
config.add_section(:redis, :url, :proxy, :sentinels, :role,
:connect_timeout, :read_timeout, :write_timeout)
:connect_timeout, :read_timeout, :write_timeout,
:async)
config.add_section(:analytics_redis, :server,
:connect_timeout, :read_timeout, :write_timeout)
config.add_section(:hoptoad, :service, :api_key)
Expand Down
18 changes: 7 additions & 11 deletions lib/3scale/backend/oauth/token_storage.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ def all_by_service_and_app(service_id, app_id, user_id = nil)
Token.from_value token, service_id, value, ttl
end
end
.force.tap do
.tap do
Copy link
Contributor

Choose a reason for hiding this comment

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

No explanation of the actual problem in the commit message. 👎

This is important, can you please explain it? I will comment below what's wrong with removing this (although I don't mean it cannot or should not be removed, depends on our usage).

# delete expired tokens (nil values) from token set
deltokens.each_slice(TOKEN_MAX_REDIS_SLICE_SIZE) do |delgrp|
storage.srem token_set, delgrp
Expand Down Expand Up @@ -185,10 +185,7 @@ def remove_whole_token_set(token_set, service_id)
# TODO: provide a SSCAN interface with lazy enums because SMEMBERS
# is prone to DoSing and timeouts
def tokens_from(token_set)
# It is important that we make this a lazy enumerator. The
# laziness is maintained until some enumerator forces execution or
# the caller calls 'to_a' or 'force', whichever happens first.
storage.smembers(token_set).lazy
storage.smembers(token_set)
Copy link
Contributor

Choose a reason for hiding this comment

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

So, the commit message is just wrong.

lazy does not magically make smembers fetch partial contents of a Redis set, process them, and then fetch more. lazy creates an Enumerator::Lazy, which fundamentally changes what happens after the call to lazy.

If storage.smembers(...) above would have been the mentioned (in TODO comments) storage.sscan(...) wrapper providing an Enumerator to have it provide something like 100 elements in each batch, lazy would still fundamentally change the way it behaves.

See, before the call to lazy:

  • SMEMBERS -> Fetch a million keys from set -> Store a million keys in an Array.
  • SSCAN -> Fetch 100 elements from set -> Store 100 elements in an Array.

After the call to lazy, imagine we have tokens_from(a_set).select { |t| t.start_with 'alex' }.take(2) elsewhere:

  • SMEMBERS w/o lazy: A million elements Array -> select iterates over all that million of elements filling in another array -> ie. creates an array with 15 elements -> returns first two matching elements in a new 2-element array.

  • SMEMBERS w/ lazy: A million elements Array -> select iterates just to find the first element matching -> take accumulates this element in an array -> select iterates to find the second element matching -> take accumulates this element in the array, now a 2-element array -> returns first two matching elements in the 2-element array.

  • SSCAN w/o lazy: 100 elements Array -> select iterates over the 100 elements filling another array -> ie. creates an array with 1 element -> returns first element in a 1-element array -> some special code would be needed to call SSCAN again and add to the results array -> another 100 elements Array is provided -> select iterates over these new 100 elements filling yet another array -> ie. creates an array with 3 elements -> take returns first two elements of the 3 -> some special code would be needed to join this 2-element array to the tail of the previous 1-element array, then trimming the array to only contain the first 2 elements.

  • SSCAN w/ lazy: 100 elements Array -> select checks elements one by one until it finds the first matching -> take accumulates the first element, but because it hasn't accumulated two, the process iterates -> select checks now remaining elements from the 100 element array one by one until it finds the second... assume the second is not found, so select iterates on its source lazy enumerator -> the SSCAN enumerator returns the next batch of 100 elements -> select gets to check now for the second matching element again -> finds it and take accumulates it in its array, a 2-element array, so it is done -> take returns the array result.

Ok, so this is to clarify, there are 2 effects at play for minimising both CPU and memory usage, which essentially correspond to external and internal fragmentation:

  1. External fragmentation: we can avoid storing millions of tokens in memory if we implement a generator/enumerator, which is what SSCAN is for (including avoiding collapsing network bandwidth and burning the Redis CPU) and we have yet to wrap it in an Enumerator.
  2. Internal fragmentation: we can avoid processing extra elements if our processing has a termination condition that does not require looking at the whole collection (ie. searching for a specific subset of the collection), and we already have this implemented with an Enumerator::Lazy.

The 1st is interesting in its own right (but we haven't yet implemented it), certainly the most interesting one. But this is removing the 2nd, and we really need to find a good reason to do so (it might be that we aren't taking advantage of the laziness, though, I just don't know at this point).

The important thing, though, if the justification exists, is that all users need to be checked in how they use the lazy enumerator to see whether they can terminate early (and if so, how big would the estimated impact be, given sets could be huge).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand how .lazy works and I made a conscious decision when I decided to remove it. Let me try to justify that. I admit that I didn't do a good job explaining this in the description of the commit.

First of all, this code is deprecated. It's only used in the endpoints that we offer to manage OAuth tokens which are deprecated.

I spent some time trying to debug the issue, but to be honest, I was not able to fully understand why .lazy does not play well with the async library. It might be related to something like this: socketry/async#23, but I do not know for sure.

Regarding your points about external/internal fragmentation:

  • External fragmentation. As you explained, .lazy() would be absolutely critical if we changed this to SSCAN as the TODO in the code mentions, but I suspect this is not going to happen. It has not happened in years, so it's not going to happen now that the code is deprecated.

  • Internal fragmentation. As far as I know, the method where the call to .lazy was is only called from OAuth::Token::Storage.all_by_service_and_app which needs to return all the tokens returned by the smembers call except for the expired ones. So there's a .select that discards some elements, but there's nothing like a take(a_few).

Copy link
Contributor

@unleashed unleashed Mar 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@davidor re: SSCAN happening - well, it can still happen and the fact this is "deprecated" does not mean SSCAN would not be used elsewhere. The fact this code implies lazy won't be usable is something that will need to be documented and investigated eventually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know. I added a note in the TODO list above to document this in the future. We might find other limitations in future PRs so I'll document all of them later.

end

def tokens_n_keys(token_set, service_id)
Expand All @@ -198,15 +195,14 @@ def tokens_n_keys(token_set, service_id)
Key.for token, service_id
end
end
# Note: this is returning two lazy enumerators

[token_groups, key_groups]
end

# Provides grouped data (as sourced from the lazy iterators) which
# matches respectively in each array position, ie. 1st group of data
# contains a group of tokens, keys and values with ttls, and
# position N of the tokens group has key in position N of the keys
# group, and so on.
# Provides grouped data which matches respectively in each array
# position, ie. 1st group of data contains a group of tokens, keys
# and values with ttls, and position N of the tokens group has key
# in position N of the keys group, and so on.
#
# [[[token group], [key group], [value_with_ttls_group]], ...]
#
Expand Down
2 changes: 1 addition & 1 deletion lib/3scale/backend/queue_storage.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ def self.connection(env, cfg)
options = Backend::Storage::Helpers.config_with(cfg.queues,
options: init_params)

Redis.new options
Storage.new(options)
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/3scale/backend/stats/storage.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ def stats_storage
def stats_storage_from_config
options = Backend::Storage::Helpers.config_with(config.analytics_redis)

Redis.new(options)
Backend::Storage.new(options)
end

def config
Expand Down
Loading