Added Rest Client Module to benchmark Restful Web Services #756

shivam-maharshi · 2016-05-28T17:03:21Z

Please review. Touched only the absolutely necessary files required. CheckStyle formatting is followed, unit tests created, build passes.

shivam-maharshi · 2016-06-01T02:25:39Z

@busbey Please give feedback when you get some time. I have incorporated all your old comments.

shivam-maharshi · 2016-06-04T20:50:37Z

@risdenk @kruthar In case you get sometime to take a look at this. Please do and provide feedback.

kruthar · 2016-06-10T02:02:00Z

Hi @shivam-maharshi - I have been looking at this PR on and off. For me it is hard to review for a couple reasons:

The PR description doesn't give any detail into what this PR is trying to accomplish, I'm hesitant to approve this because I don't know really what it is for or what exactly it does, or any of the motivation behind it, why it was necessary to create a new Workload
Following the documentation theme there isn't a README.md which would tell me a little more about the Rest driver, what it is for, how to use it, the properties, etc.

I started looking into the RestWorkload.java, it is very different from CoreWorkload.java and so is not easy to view differences between the two with a diff. This reinforces the need for more documentation around what the motivations for this PR are and the benefits it brings.

Some specific things that are confusing to me:

I am not exactly sure what this trace stuff is for. You have separated out trace properties for each operation, but then in the workload file examples you have just given the same file name for each operation. what is the point there?
It seems strange to me that at the RestWorkload.java level you have removed the scan operation.
It looks like the key and value generating paradigms have changed, especially since buildDeterministicValue doesn't exist in RestWorkload.java

Sorry for the negative feedback but some things I would really like to see are more documentation and conversation about the work you've done, at this point there is too much change to really tell everything that is going on. Along with that it would be great to discuss if the changes you have made in RestWorkload.java could actually be integrated through additions to CoreWorkload.java and the bulk of the changes in the rest binding you created.

shivam-maharshi · 2016-06-21T05:52:18Z

Hi @kruthar, thanks a lot for providing a detailed explanation. My aim is to eventually get this PR accepted. I am open to making any changes that are required to make this possible. Any feedbacks that will help me move in this direction are more than welcome! Here are the answers to some of your concerns.

I have added a detailed README.md for this driver. It includes description about what this driver is, what is the use case, how to use it and the description of the properties to be used with examples. This was an important point, thanks for pointing it out.
The main aim of this PR is to add the support of benchmarking RESTFul webservices using YCSB. Right now YCSB can only benchmark databases/KV stores, however it has the potential to be an excellent tool for benchmarking webservices mainly because of three reasons:

a. Firstly, it is highly configurable and customizable.
b. Secondly, it is capable of producing huge amount of workload without boging the client machine down, something needed for cloud services.
c. Thirdly, since it is written on java, it is platform independent.

I have performed a detailed study into the available free opensource tools most suited for benchmarking cloud webservices, and found that there is a lack of such a tool (My MS thesis is on a related topic). Hence I finally decided to add this module to YCSB since it catered to my need and I feel that many developers would be thrilled to use it. They can also easily tweak it to fit thier own usecase.

Regarding RestWorkload concerns, below are the explanations:

a. Trace is a collection of URL resources that should be hit in order to benchmark any webservice. The more realistic this collection of URL is, the more reliable and accurate are the benchmarking results because this means simulating the real life workload more accurately. Tracefile is a file that holds the trace. For example, if your webservice exists at http://{host}:{port}/{endpoint}, and you want to benchmark the performance of READS on this webservice with three resources (namely resource_1, resource_2, resource_3) then the url.trace.read file might look like this:

http://{host}:{port}/{endpoint}/resource_1
http://{host}:{port}/{endpoint}/resource_2
http://{host}:{port}/{endpoint}/resource_3

The RestWorkload will then pickup these URL resources and put them in a map. According to the requestdistribution type, the URLs will be picked from this map and passed to RestClient binding to make HTTP GET requests on these URLs. Similar logic will also apply to INSERT, DELETE and UPDATE methods.

b. In real life the URL access pattern is different for different operations. For example, HTTP GET will have a different URL access pattern than HTTP POST for a webservice. Hence there must be different trace files for each such operation. However for simplicity, we might want to use similar trace file to avoid creating different traces for different operations. Hence you see the same file name in properties file. But its just an example.

c. The support for SCAN operations are important to benchmark from a database perspective where sometimes a collection of closely located entries are fetched or updated for some application layer logic. This is a real life use case for databases. However for webservices this is a highly unlikely use case. Hence the support for this operation is not needed. However it can always be added in the future if deemed worth the time and efforts.

d. In RestWorkload, the keys generated are simply used to pick a URL from the trace maps, loaded at initialization of the class. Using this technique we pick the URLs on which next requests can be made so as to produce a URL access pattern according to the requestdistribution. The buildDeterministicValue is hence not required in this use case.

I totally understand your concerns and I am more than happy to discuss with you the reasoning behind this binding and the design opted. We can have a conversation on phone/email/skype or any other convenient option as you prefer. If you or anyone else have better suggestions regarding design and we would need to restructure the code, I would be more than happy to do that myself as well. Thanks again for your response. Hoping to hear back soon again.

kruthar · 2016-06-24T01:25:18Z

@shivam-maharshi - thanks for the background info and context, it makes a lot more sense now why this type of workload needs to be so different from the traditional CoreWorkload. I think you have also worked in enough flexibility to work with many potential REST layouts by using the trace files, a little more work for the user up front, but it means they can be sure it will hit all the endpoints they want it to and they dont seem to have to heterogenous.

@busbey - there are several things that need to be updated in this PR before being merged in, but do you have any problems with trying to get this one in for 0.10.0 as an Untested binding? I would put it in as untested to start with because of the fact that it introduces a new Workload type, and I don't have a REST services on hand to test this thing.

kruthar · 2016-06-24T01:26:39Z

rest/README.md

+The rest module will pick up URLs from the above file according to
+the `requestdistribution` property (default is zipfian) mentioned in
+the rest_workload. In the example above we assume that the property `url.prefix` 
+(see below for property description) is set to ``. If url.prefix


nit - there are two backticks here that are throwing off formatting.

Fixed it by using empty keyword instead of ``.

kruthar · 2016-06-24T01:30:43Z

Can you rebase this PR to pick up the new ycsb shell scripts, then add a line for this rest binding in bin/bindings.properties. The python tool at bin/ycsb will be deprecated out at some point, but you might as well update that also with a line for your binding.

kruthar · 2016-06-24T01:35:02Z

Would also be greatly appreciated if you could rebase your PR done to just a handful of meaningful commits. Please prefix each commit with [rest] as that is the subject matter.

shivam-maharshi · 2016-06-26T03:16:01Z

@kruthar Rebased the PR and updated bin/binding.properties and python bin/ycsb to include rest module. Also squashed all the different commits into a single meaningful commit and used [rest] in the message log. Please let me know if I need to do anything else you need from me.

kruthar · 2016-06-27T01:16:24Z

@busbey, @risdenk - looks like this is failing with the jdk8 test due to the sigar issue addressed in PR #776. I don't see any other recent PR's failing so thinking this is kinda weird. Thoughts? Is #776 required to do jdk8 builds now?

risdenk · 2016-06-28T00:59:22Z

@kruthar The sigar thing is just a warning. It doesn't actually fail anything. The real issue in the test is Killed at the end after

Connected to cluster: Test Cluster
Datacenter: datacenter1; Host: localhost/127.0.0.1; Rack: rack1
Connected to cluster: Test Cluster
Datacenter: datacenter1; Host: localhost/127.0.0.1; Rack: rack1
Connected to cluster: Test Cluster
Datacenter: datacenter1; Host: localhost/127.0.0.1; Rack: rack1

The real issue is the JVM crashed or hung or something. Maybe out of memory or something from Travis?

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:2.12.4:test (default-test) on project cassandra2-binding: Execution default-test of goal org.apache.maven.plugins:maven-surefire-plugin:2.12.4:test failed: The forked VM terminated without saying properly goodbye. VM crash or System.exit called ? -> [Help 1]

busbey · 2016-06-28T14:10:35Z

@busbey - there are several things that need to be updated in this PR before being merged in, but do you have any problems with trying to get this one in for 0.10.0 as an Untested binding? I would put it in as untested to start with because of the fact that it introduces a new Workload type, and I don't have a REST services on hand to test this thing.

I'd rather take the time to get it right before merging. 0.11.0 will come out in short order after 0.10.0.

jixuan1989 · 2016-06-29T01:50:47Z

Sorry, I am not sure that sigar is only suitable for jdk8. And if we do not have sigar files, there will be exception information in the console: no libsigar-universal64-macosx.dylib in java.library.path.

But as the above said, maybe Cassandra can still startup (I am not sure).

shivam-maharshi · 2016-06-29T15:22:34Z

@kruthar @busbey Please share your feedbacks about what all needs to be updated in the PR. I will be more than happy to work with you guys on this.

kruthar · 2016-06-30T01:10:58Z

bin/ycsb

@@ -83,6 +83,7 @@ DATABASES = {
    "orientdb"     : "com.yahoo.ycsb.db.OrientDBClient",
    "rados"        : "com.yahoo.ycsb.db.RadosClient",
    "redis"        : "com.yahoo.ycsb.db.RedisClient",
+	"rest"         : "com.yahoo.ycsb.webservice.rest.RestClient",


nit - line this up

kruthar · 2016-06-30T01:20:34Z

license edit in

bin/ycsb
distribution/pom.xml

Does anyone else have a REST service just laying around to test this with?

shivam-maharshi · 2016-06-30T02:27:09Z

@kruthar Incorporated feedbacks. For starters you can just test read operations using 5 Google URLs in the trace file like this:

url.prefix=https://www.google.com/#q=

TraceFile:
study
education
aws
cloud+computing
climate+change

To avoid getting your IP blocked you can make 1 request every 2 seconds with total operation count of 100. For full fledged testing you can install MediaWiki (PHP, Apache WebServer, MySQL) and create some sample pages. It supports Create and Delete operations too.

shivam-maharshi · 2016-07-12T03:55:19Z

@kruthar Please let me know if you need some help setting a dummy Rest service. We can work together on verifying this anytime you are free.

busbey · 2016-07-22T15:49:25Z

as an alternative for unblocking testing, what if we add an integration test that stands up an embedded REST service and then runs against it?

shivam-maharshi · 2016-07-24T00:57:12Z

@busbey We are using Wiremock for imitating Rest services (here: https://github.com/brianfrankcooper/YCSB/pull/756/files#diff-237d7ac48706b45c30b7c2ce9cec005eR97) in the RestClientTests, which take care of the HTTP handling part. It can also be used for integration test cases. On a side note I can add unit tests for RestWorkload to increase test coverage, thereby making sure that this module works as integration test cases might take sometime.

busbey · 2016-07-25T14:35:53Z

I saw the use of Wiremock for the unit tests, I was hoping for something more thorough for an integration test. Mocks are great for verifying well-encapsulated assumptions about the client, but tend to miss out on the sorts of edge cases that come from talking to an actual system. I was hoping for something like an embedded tomcat demo or the like.

NBD if there isn't something on hand. We'll just need to wait for somebody to have time to test things out. (that needn't be @kruthar btw, or even any of the existing project moderators. we accept reviews from anybody.)

shivam-maharshi · 2016-08-23T01:17:50Z

@busbey @kruthar I've added integration test cases using embedded Tomcat 8. These tests start the embedded server (once at class loading) which exposes a mock RESTFul service used for benchmarking. Benchmarking is invoked using Client class from the core module to generate load using the RestWorkload. Everything works as expected. Please review the changes and let me know.

shivam-maharshi · 2016-09-01T03:38:51Z

Moderators, I've confirmed the working of the rest-binding both manually as well as using integration test cases. Please let me know if it needs more changes to get accepted.

shivam-maharshi · 2016-09-14T04:29:43Z

@risdenk @jasontedor Can you please take a look at this pull request as the other moderators are busy.

risdenk · 2016-09-14T13:57:58Z

@shivam-maharshi I'll try to look on Friday.

shivam-maharshi · 2016-09-18T17:38:04Z

@risdenk thanks. Even though integration tests runs fine, please let me know if you need some help testing this on a real RESTFul service.

risdenk

There are some minor things that need changing and then can do another look over.

risdenk · 2016-09-19T01:13:07Z

rest/README.md

+
+    git clone git://github.com/brianfrankcooper/YCSB.git
+    cd YCSB
+    mvn -pl com.yahoo.ycsb:solr-binding -am clean package


This refers to solr-binding instead of rest?

risdenk · 2016-09-19T01:18:27Z

rest/src/test/java/com/yahoo/ycsb/webservice/rest/IntegrationTest.java

+  public final ExpectedSystemExit exit = ExpectedSystemExit.none();
+
+  private static int port = 8080;
+  private static Tomcat tomcat = new Tomcat();


Should this be in the init()?

risdenk · 2016-09-19T01:20:19Z

rest/src/test/java/com/yahoo/ycsb/webservice/rest/IntegrationTest.java

+
+  private static int port = 8080;
+  private static Tomcat tomcat = new Tomcat();
+  private static final String BASE_DIR = System.getProperty("user.dir");


You should be able to get the paths to these files with Java resources. Some psuedo code:

this.class.getClassLoader().getResource()

Thanks! Code Snippet - IntegrationTest.class.getClassLoader().getResource("error_trace.txt").getPath();

risdenk · 2016-09-19T01:21:02Z

rest/src/test/java/com/yahoo/ycsb/webservice/rest/IntegrationTest.java

+  @BeforeClass
+  public static void init() throws ServletException, LifecycleException, FileNotFoundException, IOException,
+      DBException, InterruptedException {
+    String webappDirLocation = "WebContent/";


Can WebContent be moved to test/resources?

risdenk · 2016-09-19T01:21:49Z

rest/src/test/java/com/yahoo/ycsb/webservice/rest/RestClientTest.java

+ * demonstrated.
+ */
+ // Test cases pass for JDK8 but fail for JDK7 due to WireMock.
+ @Ignore 


This whole test class is ignored?

Wiremock (The framework used to mock rest services for RestClientTest) requires JDK8 support. Hence this class is ignored as the tests fail on JDK7 but pass on JDK8.

Should be able to ignore tests for a specific JDK. See https://github.com/brianfrankcooper/YCSB/blob/master/cassandra/pom.xml#L67

That will ignore all tests (including IntegrationTests) to skip for JDK 1.7. So I skipped only RestClientTest for JDK < 1.8 only. Change: https://github.com/brianfrankcooper/YCSB/pull/756/files#diff-237d7ac48706b45c30b7c2ce9cec005eR109

risdenk · 2016-09-19T01:22:12Z

rest/src/test/java/com/yahoo/ycsb/webservice/rest/RestClientTest.java

+  @BeforeClass
+  public static void init() throws FileNotFoundException, IOException, DBException {
+    Properties props = new Properties();
+    props.load(new FileReader("src/test/resources/workload_rest"));


Again should be able to use resources loading instead of a full path?

Done. Code Snippet -
props.load(new FileReader(RestClientTest.class.getClassLoader().getResource("workload_rest").getPath()));

risdenk · 2016-09-19T01:25:08Z

core/src/main/java/com/yahoo/ycsb/workloads/RestWorkload.java

@@ -0,0 +1,300 @@
+/**


I think more time needs to be spent removing this RestWorkload file. I think it overlaps with CoreWorkload.

The format of CoreWorkload and RestWorkload of how it documents the properties and defines variables is different I think. Maybe try to make them more similar?

I agree with you that it would be nice to have RestWorkload merged into CoreWorkload but there are some concerns/points:

In order to benchmark REST-full web services we must already have URL endpoints to benchmark. This is why we have URL maps in RestWorkload. However the way core workload is written currently, it generates keys to benchmark instead of reading them from a collection of predefined keys. Which definitely makes sense for DBs but not for web services. This way DB benchmarking is inherently pretty different from web services benchmarking.

Considering the point above, if we still merge RestWorkload into CoreWorkload, it would require us to distinguish the key generation logic on the basis of some config value which would make the CoreWorkload code ugly (if-else blocks) and couple it tightly with logic specific to a particular binding.

Delete Operations are not supported in CoreWorload, they need special handling for WebServices to benchmark.

Since CoreWorkload is the central component, updating it will require an exhaustive testing for all the other bindings. This is precisely what I wanted to avoid.

To minimize code duplication RestWorkload extends CoreWorkload. Hence a lot of code is being reused and most future updates in CoreWorkload will be propagated to RestWorkload automatically thereby reducing extra code efforts.

I understand the urge to have a single workload but given the different nature of WebServices there are some inherent differences from databases which makes this task a bit non-trivial. I am open to further improve this design in the future but for now, if we can get this pull request incorporated in the master then I will create another pull request with the improved design later. I would be more than happy to discuss an improved design with you, @kruthar and @busbey.

Fair enough we can always improve it later.

risdenk · 2016-09-19T01:26:19Z

core/src/main/java/com/yahoo/ycsb/workloads/RestWorkload.java

+  private static final String UPDATE_ZIPFIAN_CONSTANT_DEAFULT = "0.99";
+  private static final String UPDATE_RECORD_COUNT_PROPERTY = "updaterecordcount";
+
+  private static Map<Integer, String> readUrlMap;


Should these all be static? Can these be accessed by different threads?

Good point! They will be accessed by different threads but only for reading. But yes it makes sense to not keep them static as a single YCSB instance can be used for multiple benchmarking with different traces. Change incorporated.

risdenk · 2016-09-19T01:27:25Z

rest/pom.xml

+    <dependency>
+      <groupId>org.apache.httpcomponents</groupId>
+      <artifactId>httpclient</artifactId>
+      <version>4.5.1</version>


For consistency, can you pull all the versions into the properties above like you did for tomcat and jersey?

risdenk · 2016-09-27T14:30:47Z

@shivam-maharshi - Can you ping this thread after you update? Thanks!

shivam-maharshi · 2016-10-08T03:20:15Z

@risdenk I have incorporated all your feedbacks except for one (Merging RestWorkload into CoreWorkload.). Please take a look. Thanks for reviewing.

shivam-maharshi · 2016-10-17T15:23:52Z

@risdenk @busbey Please let me know if we need further modifications in this PR. Thanks!

risdenk

@shivam-maharshi Sorry for the delay in getting back to this. Finally had some time to review this in some detail. Added some comments. Thanks for your patience.

risdenk · 2016-10-17T18:09:47Z

rest/pom.xml

+    <junit.version>4.12</junit.version>
+    <system-rules.version>1.16.0</system-rules.version>
+    <wiremock.version>1.57</wiremock.version>
+    <skipTests>true</skipTests>


Any reason skipTests is here?

Removed. An error during commit. Build was passing.

risdenk · 2016-10-17T18:11:11Z

rest/pom.xml

+    </dependency>
+  </dependencies>
+
+  <build>


Does this plugin need to be here?

Removed. Module builds without this plugin too.

risdenk · 2016-10-17T18:11:53Z

core/src/main/java/com/yahoo/ycsb/workloads/RestWorkload.java

@@ -0,0 +1,300 @@
+/**


Fair enough we can always improve it later.

risdenk · 2016-10-17T18:19:26Z

core/src/main/java/com/yahoo/ycsb/workloads/RestWorkload.java

+      keychooser = new ZipfianGenerator(recordCount, zipfContant);
+    } else if (requestDistrib.compareTo("latest") == 0) {
+      // TODO: To be implemented.
+      System.err.println("Latest request distribution is not supported");


This should exit instead of returning null?

The error is printed without throwing a WorkloadException

Handled - throw new WorkloadException("Latest request distribution is not supported for RestWorkload.");

risdenk · 2016-10-17T18:20:12Z

core/src/main/java/com/yahoo/ycsb/workloads/RestWorkload.java

+  private static NumberGenerator getKeyChooser(String requestDistrib, int recordCount, double zipfContant,
+      Properties p) throws WorkloadException {
+    NumberGenerator keychooser = null;
+    if (requestDistrib.compareTo("exponential") == 0) {


each of these .compareTo should just be .equals()?

May even be cleaner as a switch case

I had the same opinion but kept it consistent with the older CoreWorkload code. Updated to switch statements now.

risdenk · 2016-10-17T18:21:50Z

core/src/main/java/com/yahoo/ycsb/workloads/RestWorkload.java

+   */
+  private static synchronized Map<Integer, String> getTrace(String filePath, int recordCount)
+      throws WorkloadException {
+    Map<Integer, String> urlMap = new ConcurrentHashMap<Integer, String>();


Since this is a synchronized method does this need to be ConcurrentHashMap?

Removed both synchronized and concurrent from here since it is a read-only map and this workload method is invoked just once during client creation. Thanks for pointing out.

risdenk · 2016-10-17T18:24:06Z

rest/src/main/java/com/yahoo/ycsb/webservice/rest/RestClient.java

+    int responseCode;
+    try {
+      responseCode = httpExecute(new HttpPut(urlPrefix + endpoint), values.get("data").toString());
+    } catch (Exception e) {


This is catching all exceptions and not actually printing what happened? Would be nice to log the actual exception?

Might be able to just add the logging to the handleExceptions method

risdenk · 2016-10-17T18:26:36Z

rest/src/main/java/com/yahoo/ycsb/webservice/rest/RestClient.java

+        this.timedout.setIsSatisfied(true);
+      } catch (InterruptedException e) { 
+        // Do nothing. Kept just to remove Check-style build error.
+        byte a = 0;


Hmmm wonder if there is a better way to do this.

By updating checkstyle.xml with the below snippet a block now can have some text as opposed to just code to be considered valid:
<module name="EmptyBlock"> <property name="option" value="text"/> </module>

Link: http://checkstyle.sourceforge.net/config_blocks.html

risdenk · 2016-10-17T18:29:27Z

rest/src/test/java/com/yahoo/ycsb/webservice/rest/IntegrationTest.java

+  private static final String WORKLOAD_FILEPATH =  IntegrationTest.class.getClassLoader().getResource("workload_rest").getPath();
+  private static final String TRACE_FILEPATH = IntegrationTest.class.getClassLoader().getResource("trace.txt").getPath();
+  private static final String ERROR_TRACE_FILEPATH = IntegrationTest.class.getClassLoader().getResource("error_trace.txt").getPath();
+  private static final String RESULTS_FILEPATH = IntegrationTest.class.getClassLoader().getResource("").getPath() + "results.txt";


the getResource("") calls shouldn't be to "" they should be to the proper file or directory.

The file results doesn't exist hence we can't call getResource("results.txt");. Since we need the folder path to /src/test/resources even getResource("resources") doesn't work. Hence I've used a period - '.' signifying the current target directory.

shivam-maharshi · 2016-10-21T10:33:42Z

@risdenk I have incorporated all your feedbacks. The build is failing due to some Cassandra test cases. Can you please take a look.

Additionally, someone reached out to me asking for REST Web Services support in YCSB (https://github.com/shivam-maharshi/YCSB4WebServices/issues/1) which means it should turn out to be useful :) Let me know your feedbacks.

risdenk · 2016-10-22T00:03:15Z

@shivam-maharshi - kicked off the test again and looks like it failed related to the rest piece:

Failed tests:   insert_200(com.yahoo.ycsb.webservice.rest.RestClientTest): expected:<Status [name=OK, description=The operation completed successfully.]> but was:<Status [name=ERROR, description=The operation failed.]>

Tests run: 26, Failures: 1, Errors: 0, Skipped: 0

shivam-maharshi · 2016-10-22T00:18:21Z

@risdenk This error occurs sometime in JDK8 and sometime in JDK7 intermittently. It because some request in the RestClientTest is taking more than 10 seconds to execute and hence timer thread kills it and throws an Error Status. Right now the timeout is 10 seconds which I thought would be enough. What do you propose we do to resolve this? I am going to increase the timeout to 20 seconds and see if that passes. However in long run this is bad to build runtime. If you have any suggestions to handle this elegantly, please let me know.

shivam-maharshi · 2016-10-22T02:45:59Z

@risdenk All checks pass now. Please take a look and lemme know.

risdenk · 2016-10-24T18:58:33Z

@shivam-maharshi Travis CI for whatever reason tends to test really slowly. This seems to cause a lot of issues with Accumulo as well. Raising the timeout shouldn't hurt right? If it the test finishes then the timeout has no effect.

risdenk

@shivam-maharshi This is getting really close. One minor item related to where messages are printed. Can you also squash all your commits and make sure the one commit starts with [rest].

risdenk · 2016-10-24T19:03:00Z

rest/src/main/java/com/yahoo/ycsb/webservice/rest/RestClient.java

+
+  private int handleExceptions(Exception e, String url, String method) {
+    if (logEnabled) {
+      System.out.println(new StringBuilder(method).append(" Request: ").append(url).append(" | ")


The System.out in a few places should be System.err to make sure that messages go to stderr. I think there are a few examples of this in other bindings.

@risdenk Let me quickly fix the logging and squash all the commit with [rest] today itself. Will notify you with the next commit. Thanks!

shivam-maharshi · 2016-10-24T22:25:49Z

@risdenk Incorporated feedbacks and squashed all the commits into one. Please take a look and lemme know.

risdenk

Still a few System.out instead of System.err.

risdenk · 2016-10-25T15:23:47Z

rest/src/main/java/com/yahoo/ycsb/webservice/rest/RestClient.java

+      responseCode = handleExceptions(e, urlPrefix + endpoint, HttpMethod.POST);
+    }
+    if (logEnabled) {
+      System.out.println(new StringBuilder("POST Request: ").append(urlPrefix).append(endpoint)


System.out instead of System.err

risdenk · 2016-10-25T15:24:06Z

rest/src/main/java/com/yahoo/ycsb/webservice/rest/RestClient.java

+      responseCode = handleExceptions(e, urlPrefix + endpoint, HttpMethod.DELETE);
+    }
+    if (logEnabled) {
+      System.out.println(new StringBuilder("DELETE Request: ").append(urlPrefix).append(endpoint)


System.out instead of System.err

risdenk · 2016-10-25T15:24:10Z

rest/src/main/java/com/yahoo/ycsb/webservice/rest/RestClient.java

+      responseCode = handleExceptions(e, urlPrefix + endpoint, HttpMethod.GET);
+    }
+    if (logEnabled) {
+      System.out.println(new StringBuilder("GET Request: ").append(urlPrefix).append(endpoint)


System.out instead of System.err

@risdenk these are not error messages but just information logs. That's why I didn't update them since they will carry the messages of all the HTTP codes including success like 200, 204 etc.

I'm pretty sure any logging that isn't the measurements has usually gone to stderr. Maybe @busbey can shed some light here?

@risdenk if that's the norm we follow in YCSB then lemme make this change real quick and notify you back.

risdenk · 2016-10-25T15:24:22Z

rest/src/main/java/com/yahoo/ycsb/webservice/rest/RestClient.java

+      responseCode = handleExceptions(e, urlPrefix + endpoint, HttpMethod.PUT);
+    }
+    if (logEnabled) {
+      System.out.println(new StringBuilder("PUT Request: ").append(urlPrefix).append(endpoint)


System.out instead of System.err

shivam-maharshi · 2016-10-25T20:21:19Z

@risdenk I have updated all System.out to System.err. On of the three builds failed due to "build termination". Please take a look and lemme know. Thanks!

risdenk · 2016-10-25T20:40:23Z

@shivam-maharshi looks good to me. Thanks!

kruthar reviewed Jun 24, 2016
View reviewed changes

shivam-maharshi force-pushed the master branch from c5c48d6 to 73f4d81 Compare June 26, 2016 17:00

kruthar reviewed Jun 30, 2016
View reviewed changes

shivam-maharshi force-pushed the master branch from 042ab93 to 2be10a4 Compare August 23, 2016 00:44

risdenk requested changes Sep 19, 2016

View reviewed changes

risdenk added the db-new label Sep 27, 2016

shivam-maharshi force-pushed the master branch from 075f789 to 02480e6 Compare October 8, 2016 02:57

risdenk requested changes Oct 17, 2016

View reviewed changes

shivam-maharshi force-pushed the master branch 3 times, most recently from 6341401 to 8339804 Compare October 21, 2016 10:16

shivam-maharshi force-pushed the master branch from 8339804 to 75405a9 Compare October 22, 2016 01:04

risdenk requested changes Oct 24, 2016

View reviewed changes

shivam-maharshi force-pushed the master branch 2 times, most recently from 598b7ee to 796a031 Compare October 24, 2016 21:59

risdenk requested changes Oct 25, 2016

View reviewed changes

[rest] Added rest-binding for benchmarking RESTFull Web Services.

ef8c9f1

shivam-maharshi force-pushed the master branch from 796a031 to ef8c9f1 Compare October 25, 2016 19:49

risdenk approved these changes Oct 25, 2016

View reviewed changes

risdenk self-assigned this Oct 25, 2016

risdenk merged commit 1afb9af into brianfrankcooper:master Oct 25, 2016

risdenk mentioned this pull request Nov 15, 2016

Release version 0.12.0 #873

Closed

Added Rest Client Module to benchmark Restful Web Services #756

Added Rest Client Module to benchmark Restful Web Services #756

Conversation

shivam-maharshi commented May 28, 2016

shivam-maharshi commented Jun 1, 2016

shivam-maharshi commented Jun 4, 2016

kruthar commented Jun 10, 2016

shivam-maharshi commented Jun 21, 2016 • edited Loading

kruthar commented Jun 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kruthar commented Jun 24, 2016

kruthar commented Jun 24, 2016

shivam-maharshi commented Jun 26, 2016 • edited Loading

kruthar commented Jun 27, 2016

risdenk commented Jun 28, 2016

busbey commented Jun 28, 2016

jixuan1989 commented Jun 29, 2016

shivam-maharshi commented Jun 29, 2016

Choose a reason for hiding this comment

kruthar commented Jun 30, 2016

shivam-maharshi commented Jun 30, 2016 • edited Loading

shivam-maharshi commented Jul 12, 2016

busbey commented Jul 22, 2016

shivam-maharshi commented Jul 24, 2016

busbey commented Jul 25, 2016

shivam-maharshi commented Aug 23, 2016

shivam-maharshi commented Sep 1, 2016 • edited Loading

shivam-maharshi commented Sep 14, 2016

risdenk commented Sep 14, 2016

shivam-maharshi commented Sep 18, 2016

risdenk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

risdenk Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

shivam-maharshi Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

risdenk commented Sep 27, 2016

shivam-maharshi commented Oct 8, 2016 • edited Loading

shivam-maharshi commented Oct 17, 2016

risdenk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivam-maharshi Oct 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivam-maharshi commented Jun 21, 2016 •

edited

Loading

shivam-maharshi commented Jun 26, 2016 •

edited

Loading

shivam-maharshi commented Jun 30, 2016 •

edited

Loading

shivam-maharshi commented Sep 1, 2016 •

edited

Loading

risdenk Sep 19, 2016 •

edited

Loading

shivam-maharshi Oct 8, 2016 •

edited

Loading

shivam-maharshi commented Oct 8, 2016 •

edited

Loading

shivam-maharshi Oct 21, 2016 •

edited

Loading

shivam-maharshi commented Oct 21, 2016 •

edited

Loading