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

Dynamodb API update #592

Merged
1 commit merged into from
Jan 23, 2016
Merged

Dynamodb API update #592

1 commit merged into from
Jan 23, 2016

Conversation

yuyantingzero
Copy link
Contributor

Run workloada from an external vm to dynamodb, the performance difference between dynamodb v1 and v2 api. Results:
percentile, v1 read, v1 write, v2 read, v2 write
0.1, 34775.9, 36097, 35244, 36357
0.5, 35869.5, 36843, 36658, 37602
0.95, 39209.1, 42328.75, 41423, 43277.9
0.99, 54295.1, 68858.76, 56915, 63764.16

@yuyantingzero
Copy link
Contributor Author

@stfeng , PTAL

@ghost
Copy link

ghost commented Jan 22, 2016

Code LGTM. Thanks for doing the performance regression validation to make sure the new API does not perform worse than the old one. (Your results show they are within ~0.3% of each other)

@ghost
Copy link

ghost commented Jan 22, 2016

Please take a look at the Travis CI build error though...

@ghost
Copy link

ghost commented Jan 23, 2016

Thanks for taking care of the Travis build issue. All green now.

One final minor thing: Can you edit both of your commits to start with [DynamoDB]? We always prefix commits with [DB-binding-name], or [core](if the code is in YCSB core).

k = new Key()
.withHashKeyElement(new AttributeValue().withS(hashKeyValue))
.withRangeKeyElement(new AttributeValue().withS(key));
k = new HashMap<String, AttributeValue>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does k need to be a new hashmap here? Looks like it is initialized on line 343 already.

@risdenk
Copy link
Collaborator

risdenk commented Jan 23, 2016

@yuyantingzero does dynamodb use v1 anymore? Will these changes make testing some versions of DynamoDB not possible?

@busbey
Copy link
Collaborator

busbey commented Jan 23, 2016

as a point of clarification for @risdenk's comment, making it so we can't test earlier versions of DynamoDB isn't a blocker. It just means we have to weigh the benefit of maintaining that ability against the cost of breaking this into a new binding module (see the HBase modules or Cassandra modules as an example).

@ghost
Copy link

ghost commented Jan 23, 2016

@risdenk, the DynamoDB v1 client API has been deprecated since Apr 2013 when AWS released the 1.4.2 Java SDK. Upgrading to v2 client API is a long overdue task for this DB binding. ;)

@risdenk
Copy link
Collaborator

risdenk commented Jan 23, 2016

Great thanks @stfeng

@yuyantingzero
Copy link
Contributor Author

Thanks for the comments, @risdenk , @stfeng , @busbey . Fixed the commit message and addressed comment on line 347.

@risdenk
Copy link
Collaborator

risdenk commented Jan 23, 2016

Looks good to me 👍

@ghost
Copy link

ghost commented Jan 23, 2016

LGTM, merging.

ghost pushed a commit that referenced this pull request Jan 23, 2016
@ghost ghost merged commit 2269096 into brianfrankcooper:master Jan 23, 2016
@risdenk risdenk mentioned this pull request Feb 15, 2016
jaricftw pushed a commit to jaricftw/YCSB that referenced this pull request Jul 19, 2016
jaricftw pushed a commit to jaricftw/YCSB that referenced this pull request Jul 19, 2016
This pull request was closed.
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.

3 participants