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

Resolves Issue #57 - Add Bulk{Write, Read} to NDBench. #64

Merged
merged 3 commits into from
Feb 8, 2018

Conversation

pencal
Copy link
Contributor

@pencal pencal commented Nov 24, 2017

Resolves Issue #57

* @return a list of response codes
* @throws Exception
*/
public List<String> readBulk(final List<String> keys) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we care to implement bulk operations in the current plugins?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just retrofitting to existing plugins. Alternatively we can throw a UnsupportedOperationException.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets throw UnsupportedOperationException so that plugin owners can implement based on what they are looking for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#resolved

@ipapapa
Copy link
Contributor

ipapapa commented Nov 26, 2017

How is readBulk and writeBulk different compared to having multiple operations inside readSinlge and writeSingle? It seems that the first set would return a list of responses. How is this affecting the statistics?

@pencal
Copy link
Contributor Author

pencal commented Dec 5, 2017

@ipapapa I see the bulk to be a series of single read / write. Hence i am returning a list of responses. If we would like to see the entire batch as one unit of work, we should return one single response instead. thoughts?

* @return a list of response codes
* @throws Exception
*/
public List<String> readBulk(final List<String> keys) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

The implementation might be different in C* instead of merging single read operation. There is multiget thrift operation which is more suitable in this case, hence, I would leave the implementation to null and fix later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1. The only plugin that leverage the bulk api would be the janusgraph. i will leave the rest of the plugins as null impl and throw an exception if triggers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#resolved

List<String> responses = new ArrayList<>(keys.size());
for (String key : keys) {
String response = writeSingle(key);
responses.add(response);
Copy link
Contributor

Choose a reason for hiding this comment

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

Batch write is what we needed here.

List<String> responses = new ArrayList<>(keys.size());
for (String key : keys) {
String response = readSingle(key);
responses.add(response);
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to astyanax comments.

* @return a list of response codes
* @throws Exception
*/
public List<String> readBulk(final List<String> keys) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets throw UnsupportedOperationException so that plugin owners can implement based on what they are looking for

# Conflicts:
#	ndbench-cass-plugins/src/main/java/com/netflix/ndbench/plugin/cass/CassJavaDriverPlugin.java
@ipapapa ipapapa merged commit a6612fe into Netflix:master Feb 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants