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

Simple memcached ycsb benchmark based on aerospike ycsb benchmark #1199

Merged
merged 7 commits into from
Nov 22, 2016

Conversation

tedsta
Copy link
Contributor

@tedsta tedsta commented Nov 15, 2016

Basically the same as aerospike_ycsb benchmark, except is backed by memcached instead.

Copy link
Contributor

@yuyantingzero yuyantingzero 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 overall



def YumInstall(vm):
"""Installs the curl package on the VM."""
Copy link
Contributor

Choose a reason for hiding this comment

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

curl package :)?



def AptInstall(vm):
"""Installs the curl package on the VM."""
Copy link
Contributor

Choose a reason for hiding this comment

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

same


MEMCACHED_PORT = 11211

flags.DEFINE_integer('memcached_size', 64,
Copy link
Contributor

Choose a reason for hiding this comment

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

include unit in flag names


FLAGS = flags.FLAGS

LATEST_URL = 'http://memcached.org/latest'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest using fixed version (at least fixed major version)



def YumInstall(vm):
"""Installs the memtier package on the VM."""
Copy link
Contributor

Choose a reason for hiding this comment

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

memtier?

address = server.internal_ip
port = MEMCACHED_PORT

logging.info("Trying to connect to memcached at %s:%s" % (address, port))
Copy link
Contributor

Choose a reason for hiding this comment

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

for logging, you can do logging.info("Trying to connect to memcached at %s:%s" , address, port)

@@ -0,0 +1,121 @@
# Copyright 2014 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.

2016?

vm_groups:
servers:
vm_spec: *default_single_core
disk_spec: *default_500_gb
Copy link
Contributor

Choose a reason for hiding this comment

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

does memcached need a disk?

def StopMemcached(server):
out, _ = server.RemoteCommand(
'(echo -e "quit\n" ; sleep 1)| netcat %s %s' %
(server.internal_ip, memcached_server.MEMCACHED_PORT))
Copy link
Contributor

Choose a reason for hiding this comment

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

should be part of memcached package?

@tedsta
Copy link
Contributor Author

tedsta commented Nov 17, 2016

@yuyantingzero thanks for the review. Please take another look


FLAGS = flags.FLAGS

LATEST_URL = 'http://memcached.org/files/memcached-1.4.33.tar.gz'
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming: It is not latest anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated question to the purpose of this PR: What do you think of refactoring this URL and similar other constants related to software versions in other benchmarks to a YAML file under configs/? I can open an issue to discuss it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@meteorfox Sounds good to me

vm_util.RunThreaded(lambda f: f(), memcached_install_fns + ycsb_install_fns)
benchmark_spec.executor = ycsb.YCSBExecutor(
'memcached',
**{'memcached.hosts': memcached_vms[0].internal_ip})
Copy link
Contributor

Choose a reason for hiding this comment

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

How many memcached servers?
From the code, seems like there is no traffic between memcached servers. And this benchmark only use first memcached_vms as ycsb target.

vm_groups:
servers:
vm_spec: *default_single_core
vm_count: null
Copy link
Contributor

Choose a reason for hiding this comment

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

See comments below, how many server is used

@tedsta
Copy link
Contributor Author

tedsta commented Nov 21, 2016

@yuyantingzero could you take another look?

Copy link
Contributor

@yuyantingzero yuyantingzero left a comment

Choose a reason for hiding this comment

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

2 small suggestions, LGTM

config['vm_groups']['clients']['vm_count'] = FLAGS.ycsb_client_vms

if FLAGS['ycsb_server_vms'].present:
config['vm_groups']['servers']['vm_count'] = FLAGS.ycsb_server_vms
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently other self-managed ycsb benchmarks use --num_vms as ycsb server vms.
I like --ycsb_server_vms flag, but maybe support it in next pull request for all ycsb benchmarks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright I'll use --num_vms for this PR

vm_spec: *default_single_core
"""


Copy link
Contributor

Choose a reason for hiding this comment

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

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'll add that stuff in a followup PR. I have some more code that adds managed backends to this, and I want to provide configuration options that are equivalent across all providers.

@tedsta tedsta merged commit 37e83f8 into master Nov 22, 2016
@tedsta tedsta deleted the memcached branch November 22, 2016 23:39
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.

4 participants