Swap out spray for Akka http #111
Conversation
|
Thanks for this long needed change! It's not building on CI for some reason -- I assume it builds for you locally? |
|
It does build, and all tests run for me locally @rcoh. However, I'm using Java 8. I can revert to Akka versions that still support Java 7? Unless there are plans to migrate to Java 8 for the project? |
Codecov Report
@@ Coverage Diff @@
## master #111 +/- ##
==========================================
+ Coverage 61.2% 61.5% +0.3%
- Complexity 75 77 +2
==========================================
Files 14 15 +1
Lines 946 956 +10
Branches 232 232
==========================================
+ Hits 579 588 +9
- Misses 214 215 +1
Partials 153 153
Continue to review full report at Codecov.
|
| @@ -171,7 +179,8 @@ class AwsRequestSigner(awsCredentialsProvider: AWSCredentialsProvider, region: S | |||
| } | |||
|
|
|||
| private def hashedPayloadByteArray(httpRequest: HttpRequest): Array[Byte] = { | |||
| val data = httpRequest.entity.asString | |||
| val dataFuture = AkkaHttpUtil.entityToString(httpRequest.entity) | |||
| val data = Await.result(dataFuture, 1.second) | |||
There was a problem hiding this comment.
what precisely is being waited for here? Are we waiting for the request to finish or is it just for some sort of string conversion?
There was a problem hiding this comment.
This is an artifact of all Akka HTTP request/response entities being modeled as streams, so in this instance the full payload string is being pulled from that stream abstraction. It does not involve an HTTP request/network call.
More on streaming nature of entities: http://doc.akka.io/docs/akka-http/current/scala/http/implications-of-streaming-http-entity.html#implications-of-streaming-http-entities
| if (response.status.isFailure) { | ||
| logger.warn(s"Failure response: ${response.entity.asString.take(500)}") | ||
| val entityString = Await.result(entityFuture, 100.millis) |
There was a problem hiding this comment.
Maybe cleaner to use .recover or .recoverWith to handle the failure without having to Await on the entity?
| streamTimeout: FiniteDuration = 1000.millis) | ||
| (implicit ec: ExecutionContext = ExecutionContext.global, | ||
| materializer: ActorMaterializer): Future[String] = { | ||
| entity.toStrict(streamTimeout).map(_.data.utf8String) |
There was a problem hiding this comment.
I would prefer doing the Await here -- we wait for seemingly arbitrary amounts of time for this in other places. I'd rather standardize in one place even if it makes the API a little less flexible. Does this only return a future as a semantic side effect of akka-http or is there really something that we're waiting for?
There was a problem hiding this comment.
This future is a side effect of Akka HTTP modeling entities as streams. I'll try refactor to better consolidate necessary waiting, and think through all the implications.
|
@rcoh Regarding the java version, I have seen two PRs having problem with java 7. As a matter of fact, our infrastructure is also moving to java 8. What do you think about we upgrade the scala/akka version? I will create a PR if that sounds reasonable. |
|
@CCheSumo upgrading the scala/java version is fine with me |
|
How do you guys want to coordinate Java 8 switch with Akka versions in this PR? Java 8 would enable these changes to use stable Akka HTTP and not the old experimental jar. |
|
@seanpquig let's keep the changes in this PR. After Java 8 is enabled, we have serval libs we will upgrade, the akka core included. |
|
Sounds good @CCheSumo. Thanks. |
Apologies, didn't mean to edit your description - Russell