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

[db voltdb] Add VoltDB client #1319

Merged
merged 35 commits into from
Sep 17, 2019
Merged

Conversation

srmadscience
Copy link
Contributor

VoltDB has had a YCSB implementation for a few years now - see https://github.com/VoltDB/voltdb/tree/master/tests/test_apps/ycsb . While we supported the benchmark we'd never get round to formally integrating it with the 'official' YCSB repo. This pull request will hopefully address that.

In addition to moving the code around and fixing it so the init() method creates the schema if needed I've also made what I believe to the required changes to two pom.xml files. What I haven't been able to do is figure out how this line - which is needed - AFAIK - ends up in 'bindings.properties', as the 'bin' directory isn't in my fork:

voltdb:com.yahoo.ycsb.db.VoltClient4

I have built this on ubuntu on AWS using the Oracle JDK, and have created the schema and run workloads a - e successfully.

I am based in Dublin, Ireland and can be reached at drolfe at voltdb.com.

Regards,

DR

@srmadscience srmadscience changed the title Db voltdb [db voltdb] Jul 2, 2019
@srmadscience srmadscience changed the title [db voltdb] [db voltdb] Add VoltDB client Jul 2, 2019
Copy link
Collaborator

@busbey busbey left a comment

Choose a reason for hiding this comment

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

thanks for the effort on merging this in.

it looks like the bin directory is in your fork here:

https://github.com/srmadscience/YCSB/tree/db_voltdb/bin

don't forget to update both bindings.properties and the mapping in ycsb

voltdb/README.md Show resolved Hide resolved
voltdb/pom.xml Outdated Show resolved Hide resolved
voltdb/pom.xml Outdated Show resolved Hide resolved
voltdb/src/main/java/com/yahoo/ycsb/db/VoltClient4.java Outdated Show resolved Hide resolved
voltdb/src/main/java/com/yahoo/ycsb/db/VoltClient4.java Outdated Show resolved Hide resolved
voltdb/src/main/java/org/voltdbycsb/procs/ByteWrapper.java Outdated Show resolved Hide resolved
voltdb/src/main/java/org/voltdbycsb/procs/Put.java Outdated Show resolved Hide resolved
voltdb/src/main/java/org/voltdbycsb/procs/Scan.java Outdated Show resolved Hide resolved
voltdb/src/main/java/org/voltdbycsb/procs/ScanAll.java Outdated Show resolved Hide resolved
@srmadscience
Copy link
Contributor Author

srmadscience commented Jul 3, 2019 via email

@srmadscience
Copy link
Contributor Author

srmadscience commented Jul 16, 2019

Sean,

  1. I talked to our VP Eng and we have no issues using your licensing for the VoltDB specific YCSB code.

  2. I have made what I believe to be the 'correct' changes, and tested them to my own satisfaction. They are now in the codebase.

  3. One item of GitHub etiquette: Can I confirm it's your job to 'Resolve' comments, not mine?

Regards,

David Rolfe

@busbey
Copy link
Collaborator

busbey commented Aug 20, 2019

Hi David!

Sorry for the lag on chasing this down.

One item of GitHub etiquette: Can I confirm it's your job to 'Resolve' comments, not mine?

I think that varies by project. Personally I'm fine either way. Resolving them myself helps me get context when picking a review back up, but I am also fine just reviewing the then-current code without those markers.

I talked to our VP Eng and we have no issues using your licensing for the VoltDB specific YCSB code.

The issue isn't your VoltDB specific YCSB code (though since our project license is ALv2, we would require contributions to be under that license). The issue is that adding the Volt DB client libraries as a dependency would bring them in as a part of our binary packaging and add them as a transitive dependency should others rely on us. Having those libraries be AGPL presents a problem since it is more restrictive then the license we have for everything else. For example, any ASF projects that rely on us would have to take pains to make sure they don't get that part of the code base.

As I mentioned though, I haven't done a dependency review in some time to ensure we don't have other *GPL dependencies sitting around. I will try to find time to do that.

Is there any chance of VoltDB sticking with whatever licensing for the server code and releasing client specific libraries under ALv2 instead of AGPL? IIRC there are some other data stores that did similar, either with GPL or closed source servers that are accessed via a client that's under a permissive open source license.

@srmadscience
Copy link
Contributor Author

srmadscience commented Aug 20, 2019 via email

bin/ycsb Outdated Show resolved Hide resolved
jdbc/README.md Outdated Show resolved Hide resolved
@srmadscience
Copy link
Contributor Author

Sean,

Spoke to our VP/Eng. We're looking at what's involved in changing to Apache for the client. In the mean time I'll work on the other items...

DR

@srmadscience
Copy link
Contributor Author

Sean,

We can fix it so the client uses the MIT licence. Will that work for you guys?

David Rolfe

@busbey
Copy link
Collaborator

busbey commented Aug 28, 2019

Yeah, that'd be great.

@srmadscience
Copy link
Contributor Author

As far as I can tell We've fixed all the issues that have been raised so far. Can someone review? Thanks!

Copy link
Collaborator

@busbey busbey left a comment

Choose a reason for hiding this comment

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

I think we're getting close!

I also plan to download the client jar to make sure the license metadata in it matches the pom.

Do y'all have plans to call out the license change on the client jar anywhere?

@srmadscience
Copy link
Contributor Author

I've hit an issue I need some advice on. If I change the VoltDB lib to runtime or test dependency the build fails because it sees the stored procedure classes, tries to compile them, and fails because the server libs aren't in the path. My thinking is to dynamically write the stored procedure classes - i.e. they would cease to exist as Java classes and turn into Strings that are used to compile classes when we have determined we are connected to a real, live copy of VoltDB. Is this the 'least worst' solution to this problem?

@busbey
Copy link
Collaborator

busbey commented Sep 6, 2019

Try provided scope? that scope means "I need this to compile and test, but at runtime it'll be present in the environment". I think that's the intention?

@srmadscience
Copy link
Contributor Author

'provided' worked.

@srmadscience
Copy link
Contributor Author

As far as I can tell everything raised on this pass has been done...

pom.xml Outdated
@@ -110,6 +110,7 @@ LICENSE file.
<thrift.version>0.8.0</thrift.version>
<voldemort.version>0.81</voldemort.version>
<tablestore.version>4.8.0</tablestore.version>
<voltdb.version>9.1</voltdb.version>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought it was specifically version 9.1.1 we needed? Also it doesn't look like the voltdb-binding pom is using this currently?


int ratelimit = strLimit != null ? Integer.parseInt(strLimit) : Integer.MAX_VALUE;
try {
mclient = ConnectionHelper.createConnection(Thread.currentThread().getId(), servers, user, password, ratelimit);
Copy link
Collaborator

Choose a reason for hiding this comment

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

if clientId isn't needed anymore, could it get removed? this call here is still not guaranteed to pass a unique long for each DB instance, since a thread could get reused.

private static boolean haveDb = false;

@BeforeClass
public static void setup() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should be able to do a single Assume.assumeTrue(haveDb); at the end of this setup method instead of doing one on each test.

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 tried this and got:

Tests run: 2, Failures: 0, Errors: 2, Skipped: 0, Time elapsed: 0.439 sec <<< FAILURE! - in com.yahoo.ycsb.db.voltdb.test.VoltDBClientTest
com.yahoo.ycsb.db.voltdb.test.VoltDBClientTest Time elapsed: 0.439 sec <<< ERROR!
org.junit.AssumptionViolatedException: got: , expected: is
at org.junit.Assume.assumeThat(Assume.java:95)
at org.junit.Assume.assumeTrue(Assume.java:41)
at com.yahoo.ycsb.db.voltdb.test.VoltDBClientTest.setup(VoltDBClientTest.java:100)

com.yahoo.ycsb.db.voltdb.test.VoltDBClientTest Time elapsed: 0.439 sec <<< ERROR!
org.junit.AssumptionViolatedException: got: , expected: is
at org.junit.Assume.assumeThat(Assume.java:95)
at org.junit.Assume.assumeTrue(Assume.java:41)
at com.yahoo.ycsb.db.voltdb.test.VoltDBClientTest.teardown(VoltDBClientTest.java:124)

I've put it back the way it was - do you know how to get it to work with a single 'Assume' or shall we leave it as is?

@srmadscience
Copy link
Contributor Author

Other than the "Assume.assumeTrue(haveDb)" niggle everything appear to be OK now...

@busbey
Copy link
Collaborator

busbey commented Sep 17, 2019

Yep, I agree. Thanks for keeping at this David!

@busbey busbey merged commit cc165f2 into brianfrankcooper:master Sep 17, 2019
@busbey busbey mentioned this pull request Sep 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants