[memcached] Added memcached binding.

[mbrukman]

The memcached support was extracted from PR #98 by @jbellis, with cleanups to
bring it in line with current APIs and style guide.

This PR also addresses issue #326.

@busbey, could you please review? Thanks!

[kruthar]

Can you expand a bit on 1 and 2? Maybe just a link to a relevant memcached installation page, and possible a copy and paste of Java and Maven installation instructions from the mongodb module: https://github.com/brianfrankcooper/YCSB/tree/master/mongodb

[mbrukman]

Done for step 1. For step 2, it seems like this is binding-agnostic, so presumably it should be documented in a single place, and every binding can just point to it.

In fact, even step 5 is the same for everything, just substituting the binding is sufficient.

What are your thoughts about moving the "install Java / Maven" to a common place and linking to it? If so, where should it be located: top-level README, perhaps?

[mbrukman]

To make progress on this PR, I've added a link to ../mongodb/README.md and refer the user to follow step 2 in that doc. Let's consider moving Java / Maven instructions to a common place as a separate PR?

[kruthar]

That should be ok for now. I agree it should probably go into a single documented place. The top level README is an option.

[mbrukman]

OK, happy to do that as a follow-up PR. Anything else you'd like to see as part of this PR, or is it good to go now?

[mbrukman]

PR 523 moves Java + Maven instructions from MongoDB's README to the top-level README.

[mbrukman]

@kruthar, thanks for the review! I think I've addressed all of your comments; please take another look.

[kruthar]

uh-oh, looks like you might have rebased farther then you should have, you've replayed several commits. It looks like you should only have a single commit, but now you have 5.

[mbrukman]

Rebased on master, re-pushed, back to a single commit. Sorry about that!

[kruthar]

Last thing - It might be good to add a note in the README about how to add another MemcachedClient if someone ever wants to do so, and explain which memcached implementations the current client supports (I assume this is why you created the abstract class).

Other then that, I pulled, built and did some very basic testing and things look good to me.

A second opinion before merge would be great.

[mbrukman]

The base class was there with the original PR so I left it in, under the assumption that either the original author was going to use it, or there are other parts to the original PR that may want to use it, and I wanted to be as close to original PR to make it easy to do the large merge that @busbey is working on.

If it were up to me, I would probably allow using different Memcache client implementations such as xmemcached instead of Spymemcached, but that would require a slightly different class structure (right now the signature is hardcoded to return a Spymemcached client).

[mbrukman]

@kruthar, @busbey — I think I see what the issue here is: the YCSB style is to call every binding's main class [software]Client, which means this one should be called MemcachedClient. Spymemcached, the actual memcached client library, has a class by the same name. These classes cannot co-exist in the same Java file, since Java does not allow aliases or typedefs, so we have to split them up somehow.

Here are some alternatives I've come up with to make progress on this; let me know which you would prefer.

Option 0.

Keep things as they are in this PR.

Pros:

• already done :-)

Cons:

• two classes

Option 1.

Keep the main class in YCSB as MemcachedClient, wrap the Spymemcached class in another class.

Cons:

• still two classes

Option 2.

Rename the main class in YCSB to, e.g., MemcacheBenchmark, use the MemcachedClient from Spymemcached as-is.

Pros:

• single Java file

Cons:

• breaks tradition with other YCSB bindings

Option 3.

Rewrite this to enable substituting alternative implementations to Spymemcached: create an abstract interface, a factory class, and return Spymemcached as the only (current) implementation, but leave the possibility of adding another implementation backed by a different memcached client.

Pros:

• actually uses multiple classes to achieve something

Cons:

• even more than two files
• non-trivial amount of additional work
• not clear if anyone would benefit from any other clients than Spymemcached: to my knowledge, it's the most popular Java client for memcached

[busbey]

the two classes are in different packages, so you can just use them as-is and refer to the one from spymemcached by its fully qualified name.

[mbrukman]

@busbey — yes, but that only works if using wildcard imports:

import net.spy.memcached.*;
public class MemcachedClient extends DB {

but explicit import does not work:

import net.spy.memcached.MemcachedClient;
public class MemcachedClient extends DB {

and errors out with:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.3:compile (default-compile) on project memcached-binding: Compilation failure
[ERROR] .../ycsb/memcached/src/main/java/com/yahoo/ycsb/db/MemcachedClient.java:[49,1] com.yahoo.ycsb.db.MemcachedClient is already defined in this compilation unit

I was under the impression that wildcard imports is bad style:

• Stack Overflow discussion
• Google's Java style guide
• OpenJDK style guide

and assumed that checkstyle will disallow it (turns out, YCSB's usage of checkstyle does not, even though it's supported).

It turns out that other bindings in YCSB already have wildcard imports of the following variety:

import java.net.*;
import java.util.*;

which suggests that it's OK to do in YCSB.

I've updated my PR to selectively use wildcard imports just to address this issue. Please take a look.

[busbey]

It looks like you're already using the fully qualified name for the spymemcached, so importing it shouldn't be necessary at all.

[mbrukman]

@busbey — hmmm, I didn't realize that would work. Thanks! Done.

[kruthar]

Both of these repositories are not used and can be removed. Probably left over from a copy paste, or maybe from deciding which JSON framework to use.

[mbrukman]

Removed.

[mbrukman]

@busbey, I think this PR is ready to merge. Please let me know if you have any other comments.

[cmccoy]

@kruthar / @busbey - any other comments? This looks good to me.

[kruthar]

Nothing more from me. 👍

[busbey]

I was planning to do another pass this evening to check for things. The only bits that I remember was the comment about containing Google copyrighted code (but not seeing any in the patch) and the copyright year since this was based on a prior PR.

[mbrukman]

@busbey – Turns out, I labeled all files as "Copyright (c) 2015 YCSB contributors." even for the new files that I created, such as package-info.java and README.md; I've just updated those to be "Copyright (c) 2015 Google Inc." to be correct. That said, Google legal also told me that Google Inc. owns the copyright for the modifications that I made to the original PR, so even if it's not explicitly listed as the copyright owner for the entire file, they believe that Google Inc. should be listed in the NOTICE file.

Regarding the copyright year, I modified the copyright line to 2014-2015 for the files that came from the original PR: MemcachedClient.java and pom.xml.

[busbey]

Normally we attribute copyright for changes via the author on git commits, and note it on the files via the "YCSB contributors" line. Generally, the project would prefer if Google could be covered under the banner of YCSB contributors.

If they're not, then putting them in the NOTICE file is fine so long as downstream folks can figure out which changes are attributed to Google.

[busbey]

is the year range here correct?

[mbrukman]

You're right, this was not created in 2014. Updated to 2015, also updated it to be "YCSB contributors" per discussion below.

[busbey]

If we need to include attribution to Google on a per-file basis, then the files that are a mixture of old changes and new need to reflect both, similar to how we handle having both the old Yahoo! specific line and the contributors line on hte top-level README:

https://raw.githubusercontent.com/brianfrankcooper/YCSB/master/README.md

That said, it'd be much simpler for everyone involved if Google legal was fine with their rights being covered under "YCSB contributors".

If they aren't and we need some explicit mention, the difficulty in listing things just in NOTICE is there's no way for a downstream consumer who doesn't have access to git to turn even "contributions from @mbrukman (c) 2015 Google, Inc" into actual sections of code.

[cmccoy]

@mbrukman - could you just add Google, Inc to NOTICE.txt in the root of the repository and leave the copyrights as "YCSB contributors" in the headers?

[busbey]

wfm. maybe we can make a follow on to add a section in the NOTICE that explains "YCSB Contributors" with a link to a page that explains how to find people via git history.

[mbrukman]

I've updated the copyright headers to be Copyright (c) <YEAR> YCSB contributors. as agreed to above. I've fixed the years per above comments for files I created in 2015 that were not part of the original PR that this change is based on.

Also, the easy way of finding contributions that are (c) Google Inc. would be to look for commits by users with @google.com email addresses, which is more correct than saying "all contributions from GitHub user xyz are (c) Company Inc." because not all contributions under a single GitHub user id are owned by a single corporation.

[busbey]

Also, the easy way of finding contributions that are (c) Google Inc. would be to look for commits by users with @google.com email addresses, which is more correct than saying "all contributions from GitHub user xyz are (c) Company Inc." because not all contributions under a single GitHub user id are owned by a single corporation.

That's a really good point.

[mbrukman]

Thank you @busbey, @kruthar, and @cmccoy for the reviews and feedback!