Instantly share code, notes, and snippets.

Embed
What would you like to do?
package com.twitter.finagle.httpx
import com.twitter.finagle._
import com.twitter.util.{Await, Future}
import org.junit.runner.RunWith
import org.scalatest.junit.JUnitRunner
import org.scalatest.{FunSuite, ShouldMatchers}
import scala.compat.Platform
@RunWith(classOf[JUnitRunner])
class NackTest extends FunSuite with ShouldMatchers {
test("Serving a NACK triggers a retry in the default HTTP Client") {
var counter = new AtomicInteger(0)
val service = new Service[Request, Response] {
def apply(request: Request): Future[Response] = {
// Magical 4, if I change this to 5 the client fails, something
// to do with FailureAccrual or is it tied to another setting?
if (counter.getAndIncrement < 50) {
val resp = Response(Status.ServiceUnavailable)
resp.headers().add("finagle-http-nack", Unit)
Future.value(resp)
// Or Future.exception(Failure.reject("bar"))
}
else {
Future.value(Response(Status.Ok))
}
}
}
def logger(str: String): Filter[Request, Response, Request, Response] = Filter.mk { (req, next)
// When printing the time I see there is no backoff when connecting
// to the same instance, is that by design?
println(s"$str received $req at ${Platform.currentTime}")
next(req)
}
val server1 = Httpx.serve(":0", logger("server 1") andThen service)
val server2 = Httpx.serve(":0", logger("server 2") andThen service)
val client = Httpx.newService(Name.bound(server1.boundAddress, server2.boundAddress), "nack-client")
val response = client(Request("/foo"))
Await.result(response).getStatusCode() should be(200)
Await.all(server1.close(), server2.close())
}
}
@mosesn

This comment has been minimized.

mosesn commented Sep 12, 2015

the counter isn't threadsafe–can you make counter an AtomicInteger?

Regarding the behavior, I'm not sure what's going on.

Failure accrual should be per-host, and you need 5 failures in a row for failure accrual to kick in, so it should only work if:
A. the non-threadsafe int is just never updated on the other threads (possible? but I think finagle has enough synchronization that it should sort of work even without good threadsafety)
B. we hit each host five times in a row

Requeueing doesn't have backoffs, so I don't know why you would see backoffs for either endpoint.

@mosesn

This comment has been minimized.

mosesn commented Sep 12, 2015

As an aside, for these kinds of features, we shouldn't have to write manual tests for every protocol, so it would be neat to have some tooling for writing these kinds of tests in a protocol-agnostic way.

@mosesn

This comment has been minimized.

mosesn commented Sep 12, 2015

OK, so I tried running it locally, and I'm actually seeing the expected behavior. There's no backoff on either endpoint, and you can scale the counter size up to 10, which is when we can pigeonhole that each host will be failure accrual-ed. Was your comment for when you were only talking to one endpoint?

@spockz

This comment has been minimized.

Owner

spockz commented Sep 15, 2015

Ah. I didn't receive a notification of your messages. I'll look into your comments today. I also want to move this to some kind of testkit function. The idea is that someone then can use that function to check whether his custom configured client still does retry on nack.

@spockz

This comment has been minimized.

Owner

spockz commented Sep 16, 2015

Requeueing doesn't have backoffs, so I don't know why you would see backoffs for either endpoint.

That's exactly what I mean. I suppose you don't want to flood servers due to retry after NACK.

As an aside, for these kinds of features, we shouldn't have to write manual tests for every protocol, so it would be neat to have some tooling for writing these kinds of tests in a protocol-agnostic way.

I suppose this test it rather protocol agnostic when you make Request and Response type parameters and use Future.exception(Failure.reject("bar")).

Was your comment for when you were only talking to one endpoint?

The comment was when I tested with a single endpoint. Adding the AtomicInteger doesn't change much.

As far as prior art goes, cf https://github.com/twitter/finagle/blob/develop/finagle-mux/src/test/scala/com/twitter/finagle/mux/EndToEndTest.scala#L135

Ah, nice to see that I had more or less the same idea. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment