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

Improved performance #5

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Added 'id' param to all Ariadne 'get' methods
- 'id' param will help improving the overall performance of application
as the user will be able to fetch the required data directly by passing the
'id' i.e. previously used to store the data.
- new TCs written for modified methods

added keyword arguments for 'get' methods

added 'keyword arguments' & 'highredis' functionality
  • Loading branch information
Swapnil committed Jun 23, 2016
commit e181482ac7116f679586e28a5ab4b096cda06542
2 changes: 2 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ PATH
specs:
ariadne (0.0.1)
fakeredis
hiredis
redis

GEM
Expand All @@ -15,6 +16,7 @@ GEM
docile (1.1.5)
fakeredis (0.5.0)
redis (~> 3.0)
hiredis (0.6.1)
json (1.8.3)
method_source (0.8.2)
minitest (5.8.4)
Expand Down
1 change: 1 addition & 0 deletions ariadne.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,6 @@ Gem::Specification.new do |spec|
spec.add_development_dependency 'bundler'
spec.add_development_dependency 'rake'
spec.add_runtime_dependency 'redis'
spec.add_runtime_dependency 'hiredis'
spec.add_runtime_dependency 'fakeredis'
end
30 changes: 15 additions & 15 deletions lib/ariadne/ariadne_util.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,30 +2,35 @@
require 'json'
require 'time'
require 'redis'
require 'redis/connection/hiredis'

module Ariadne
DataUtil.init_redis_cli(Redis.new(url: ENV['REDIS_URL']))
def self.get_app_name(app_name: nil)
app_name ||= ENV['APP_NAME']
app_name || ''
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the above two lines can be written like this

app_name ||= ( ENV['APP_NAME'] || '' )


DataUtil.init_redis_cli(redis_obj: Redis.new(url: ENV['REDIS_URL']))

def self.insert_data(options = {})
def self.insert_data(options: {}, app_name: nil)
options[:id] ||= options['id']
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this line for?

options[:app_name] ||= options['app_name']
raise 'Please specify data to be inserted for Ariadne.insert_data method!' if options.size <= 0
raise 'Please specify id to be passed for Ariadne.insert_data method!' if options[:id].nil? || options[:id].size <= 0
DataUtil.insert_data_in_redis(options.merge!(app_name: get_app_name(options[:app_name])))
raise 'Please specify data to be inserted for Ariadne.insert_data method!' if options.empty?
raise 'Please specify id to be passed for Ariadne.insert_data method!' if options[:id].nil? || options[:id].size == 0
DataUtil.insert_data_in_redis(options: options, app_name: get_app_name(app_name: app_name))
rescue StandardError => e
puts e
e
end

def self.get_data(id = nil, app_name = '')
DataUtil.get_data_from_redis(id, get_app_name(app_name))
def self.get_data(id: nil, app_name: nil)
DataUtil.get_data_from_redis(id: id, app_name: get_app_name(app_name: app_name))
rescue StandardError => e
puts e
e
end

def self.get_data_with_time_difference(id = nil, app_name = '')
redis_data = get_data(id, get_app_name(app_name))
def self.get_data_with_time_difference(id: nil, app_name: nil)
redis_data = get_data(id: id, app_name: app_name)
default_time_diff_threshold = 30
output_data = []
unless redis_data.nil?
Expand All @@ -42,9 +47,4 @@ def self.get_data_with_time_difference(id = nil, app_name = '')
end
output_data.to_json
end

def self.get_app_name(app_name = '')
app_name = ENV['APP_NAME'] if app_name.nil? || app_name.size <= 0
app_name || ''
end
end
10 changes: 5 additions & 5 deletions lib/ariadne/data_util.rb
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@

module DataUtil
def self.init_redis_cli(obj = nil)
@redis_cli = obj
def self.init_redis_cli(redis_obj: nil)
@redis_cli = redis_obj
Copy link
Contributor

Choose a reason for hiding this comment

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

should this not be in a begin-rescue-end block? given that redis_obj can be nil

end

def self.get_data_from_redis(id, app_name)
def self.get_data_from_redis(id:, app_name:)
key = id.nil? ? "#{app_name}*" : "#{app_name}:#{id}"
keys = @redis_cli.keys key
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure why I dont like the way this has been written

The caller of this method already knows if id is nil and hence can pass suitable value for key. Why do we need to parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is not possible for the caller to know the key!
This condition is there because if no id is specified i.e. if the id is nil then the caller must get all records related to that app.
We can't let the caller directly pass the key.
Here we are generating a required key.

raise "Data not available for #{app_name}!" if keys.empty?
Expand All @@ -15,8 +15,8 @@ def self.get_data_from_redis(id, app_name)
puts e
end

def self.insert_data_in_redis(options = {})
key = "#{options[:app_name]}:#{options[:id]}"
def self.insert_data_in_redis(options:, app_name:)
key = "#{app_name}:#{options[:id]}"
options[:time] = Time.now
redis_data = nil
@redis_cli.pipelined do
Expand Down
2 changes: 1 addition & 1 deletion lib/ariadne/testing.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
require_relative 'data_util'
require 'fakeredis'

DataUtil.init_redis_cli(Redis.new)
DataUtil.init_redis_cli(redis_obj: Redis.new)
50 changes: 21 additions & 29 deletions test/test_ariadne.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,95 +4,87 @@ def test_insert_data
app_name = 'test'
data = {
id: 123,
state: 'active',
app_name: app_name
state: 'active'
}

out = Ariadne.insert_data(data)
out = Ariadne.insert_data(options: data, app_name: app_name)
assert_equal Redis::Future, out.class
end

def test_get_data
app_name = 'test'
data = {
id: 123,
state: 'active',
app_name: app_name
state: 'active'
}

Ariadne.insert_data(data)
out = Ariadne.get_data(nil, app_name)
Ariadne.insert_data(options: data, app_name: app_name)
out = Ariadne.get_data(app_name: app_name)
assert_equal JSON.parse(out[0])['id'], data[:id]
end

def test_get_data_with_time_diff_without_delay_should_be_empty
app_name = 'test'
data = {
id: 123,
state: 'active',
app_name: app_name
state: 'active'
}

Ariadne.insert_data(data)
out = Ariadne.get_data_with_time_difference(nil, app_name)
Ariadne.insert_data(options: data, app_name: app_name)
out = Ariadne.get_data_with_time_difference(app_name: app_name)
assert_empty JSON.parse(out)
end

def test_get_data_with_time_diff_with_delay_should_not_be_empty
app_name = 'test'
app_name = 'test'
delay_interval = 2
data = {
id: 123,
state: 'active',
app_name: app_name,
delay_interval: delay_interval
}

Ariadne.insert_data(data)
Ariadne.insert_data(options: data, app_name: app_name)
sleep delay_interval
out = Ariadne.get_data_with_time_difference(nil, app_name)
out = Ariadne.get_data_with_time_difference(app_name: app_name)
assert_equal JSON.parse(out)[0]['id'], data[:id]
end

def test_get_data_with_id
app_name = 'test'
data = {
id: 123,
state: 'active',
app_name: app_name
state: 'active'
}

Ariadne.insert_data(data)
out = Ariadne.get_data(data[:id], app_name)
Ariadne.insert_data(options: data, app_name: app_name)
out = Ariadne.get_data(id: data[:id], app_name: app_name)
assert_equal JSON.parse(out[0])['id'], data[:id]
end

def test_get_data_with_time_diff_with_id_without_delay_should_be_empty
app_name = 'test'
data = {
id: 123,
state: 'active',
app_name: app_name
state: 'active'
}

Ariadne.insert_data(data)
out = Ariadne.get_data_with_time_difference(data[:id], app_name)
Ariadne.insert_data(options: data, app_name: app_name)
out = Ariadne.get_data_with_time_difference(id: data[:id], app_name: app_name)
assert_empty JSON.parse(out)
end

def test_get_data_with_time_diff_with_id_with_delay_should_not_be_empty
app_name = 'test'
app_name = 'test'
delay_interval = 2
data = {
id: 123,
state: 'active',
app_name: app_name,
delay_interval: delay_interval
}

Ariadne.insert_data(data)
Ariadne.insert_data(options: data, app_name: app_name)
sleep delay_interval
out = Ariadne.get_data_with_time_difference(data[:id], app_name)
out = Ariadne.get_data_with_time_difference(id: data[:id], app_name: app_name)
assert_equal JSON.parse(out)[0]['id'], data[:id]
end
end
end