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

Generic Key generators #214

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vinaykumarchella
Copy link
Contributor

First attempt to make Key generators more generic instead of just Strings

@vinaykumarchella
Copy link
Contributor Author

@amcp Can you review this?

@vinaykumarchella
Copy link
Contributor Author

This PR is in very initial stages, basically brainstorming stage, not close to the merge stage.

@amcp
Copy link
Contributor

amcp commented Oct 11, 2018

taking a look

Copy link
Contributor

@amcp amcp left a comment

Choose a reason for hiding this comment

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

minor comments and questions

@@ -32,7 +32,7 @@
*
* @author vchella, pencal
*/
public interface NdBenchAbstractClient<W> {
public interface NdBenchAbstractClient<K, W> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please document the addition of parameter K. Also, is it OK not to narrow down the expected superclass/interfaces for K?

@@ -32,7 +32,7 @@
*
* @author vchella, pencal
*/
public interface NdBenchAbstractClient<W> {
Copy link
Contributor

Choose a reason for hiding this comment

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

while we are in here, lets discuss the naming of parameter W. W stands for write because it is the type returned by writeSingle but it is not descriptive of what the type is. Can we rename W to PostImageType or am I thinking about it too much?

@@ -0,0 +1,20 @@
package com.netflix.ndbench.core;
Copy link
Contributor

Choose a reason for hiding this comment

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

add a license header


import com.netflix.ndbench.api.plugin.NdBenchMonitor;

public interface NdBenchOperation<K> {
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a classdoc. also, is there some functional interface we could make this interface extend? like for example Function or BiFunction?

boolean isReadType();

boolean isWriteType();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

add a newline at the end of file

import com.netflix.ndbench.api.plugin.NdBenchMonitor;

public interface NdBenchOperation<K> {
boolean process(NdBenchDriver driver,
Copy link
Contributor

Choose a reason for hiding this comment

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

please add javadoc on the spec

@@ -340,7 +340,7 @@ public Response stop() throws Exception {
public Response readSingle(@PathParam("key") String key) throws Exception {

try {
String value = ndBenchDriver.readSingle(key);
String value = "";//ndBenchDriver.readSingle(key);
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these calls commented out?

private static final Logger logger = LoggerFactory.getLogger(InMemoryTestPlugin.class);

private final Map<String, String> data = Maps.newConcurrentMap();
private final Map<K, String> data = Maps.newConcurrentMap();

private DataGenerator dataGenerator;
private static final String ResultOK = "Ok";
Copy link
Contributor

Choose a reason for hiding this comment

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

use all caps for a string constant?

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