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

Added managed memcache service and an AWS ElastiCache backend #1235

Merged
merged 1 commit into from Apr 3, 2017

Conversation

tedsta
Copy link
Contributor

@tedsta tedsta commented Dec 6, 2016

This adds an abstraction over managed memcached services and implements an ElastiCache backend. It also adds a managed scenario to the memcached ycsb benchmark

TL;DR you can now run memcached ycsb against ElastiCache.

@tedsta tedsta changed the title [WIP] Added managed memcache service an an AWS ElastiCache backend [WIP] Added managed memcache service and an AWS ElastiCache backend Dec 7, 2016
@tedsta tedsta changed the title [WIP] Added managed memcache service and an AWS ElastiCache backend Added managed memcache service and an AWS ElastiCache backend Mar 7, 2017
@tedsta
Copy link
Contributor Author

tedsta commented Mar 7, 2017

@gareth-ferneyhough @yuyantingzero PTAL. Note this involves a small change to AwsFirewall

Copy link
Contributor

@gareth-ferneyhough gareth-ferneyhough left a comment

Choose a reason for hiding this comment

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

Looks good, just a few nitpicks for fun.

benchmark_spec.always_call_cleanup = True

if FLAGS.memcached_managed == providers.GCP:
raise Exception("GCP managed memcached backend not implemented yet")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use NotImplementedError

if FLAGS.memcached_managed == providers.GCP:
raise Exception("GCP managed memcached backend not implemented yet")
elif FLAGS.memcached_managed == providers.AWS:
ycsb_backend = 'memcached'
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe make this string a constant at the top of file since it is used multiple times.

@@ -0,0 +1,35 @@
# Copyright 2016 PerfKitBenchmarker Authors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

update to 2017

@@ -0,0 +1,135 @@
# Copyright 2016 PerfKitBenchmarker Authors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

2017

def Create(self):
# Open the port memcached needs
aws_network.AwsFirewall.GetFirewall() \
.AllowPortInSecurityGroup(self.region, self.security_group_id, 11211)
Copy link
Contributor

Choose a reason for hiding this comment

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

Make port number (11211) a constant

cmd = ['aws', 'elasticache', 'delete-cache-cluster',
'--cache-cluster-id=%s' % self.cluster_id,
'--region=%s' % self.region]
out, _, _ = vm_util.IssueCommand(cmd)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you aren't using out, do you need to save it and its underscore friends?

@@ -0,0 +1,39 @@
# Copyright 2016 PerfKitBenchmarker Authors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

2017

@tedsta tedsta force-pushed the elasticache branch 2 times, most recently from 4c07286 to 1b4dd7c Compare April 3, 2017 19:41
@gareth-ferneyhough
Copy link
Contributor

LGTM

@tedsta tedsta merged commit 1329b47 into master Apr 3, 2017
@bvliu bvliu deleted the elasticache branch January 7, 2020 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants