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

finagle-http not sending enough requests #148

Closed
vhotspur opened this issue Jun 12, 2019 · 1 comment · Fixed by #156
Closed

finagle-http not sending enough requests #148

vhotspur opened this issue Jun 12, 2019 · 1 comment · Fixed by #156
Labels
bug Something isn't working
Milestone

Comments

@vhotspur
Copy link
Member

Looking at the following code in benchmarks/twitter-finagle/src/main/scala/org/renaissance/twitter/finagle/FinagleHttp.scala it seems to me that val response ... should be moved to the inner loop as in the current setup we sent one request per client and repeatedly read its result. But the naming suggests that there should be NUM_REQUESTS requests sent.

Is this a bug or intentional behaviour?

override def run(): Unit = {
  val client: Service[http.Request, http.Response] =
    com.twitter.finagle.Http.newService(s"localhost:$port")
  val request = http.Request(http.Method.Get, "/json")
  request.host = s"localhost:$port"
  val response: Future[http.Response] = client(request) // <============
  for (i <- 0 until NUM_REQUESTS) {
    Await.result(response.onSuccess { rep: http.Response =>
      totalLength += rep.content.length
    })
  }
  client.close()
}
@axel22
Copy link
Member

axel22 commented Jun 13, 2019

Indeed, this looks like a bug. We should rewrite that part.

@farquet farquet added the bug Something isn't working label Jun 13, 2019
@farquet farquet added this to the 1.0.0 milestone Jun 13, 2019
vhotspur added a commit that referenced this issue Jun 17, 2019
finagle-http is now sending requests in parallel. Also, the measured
part of the benchmark now excludes starting of the threads (there is a
barrier to start all the work at once).

Note that the parameters (number of clients and number of requests) were
changed to roughly correspond to the previous performance (same order of
magnitude).

Also note that this commit changes the performance of the finagle-http
benchmark drastically.

For references, here are the issues summarized:

 * Issue 148: single requests was repeatedly waited for instead of
   sending the whole request again.
 * Issue 149: no parallel requests were actually executed.
vhotspur added a commit that referenced this issue Jun 26, 2019
finagle-http is now sending requests in parallel. Also, the measured
part of the benchmark now excludes starting of the threads
(there is a barrier to start all the work at once).

Note that the parameters (number of clients and number of requests)
were changed to roughly correspond to the previous performance
(same order of magnitude).

Also note that this commit changes the performance of the
finagle-http benchmark drastically.

This closes #148, #149 and #161.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants