Exit JVM if an error is thrown or the main loop exits #119
Conversation
Fixed issues with PodIds containing . Exit JVM if an error is thrown or the main loop exits
| @@ -33,4 +33,17 @@ akka { | |||
| # outgoing connection might be closed if no call is issued to Mesos for a while | |||
| idle-timeout = infinite | |||
| } | |||
| } | |||
|
|
|||
| stream { | |||
There was a problem hiding this comment.
As a library, I feel like we shouldn't rely on putting global configuration here for Akka. We should instead be adding the appropriate catch-all error catching (which will expose exceptions properly).
There was a problem hiding this comment.
Yeah, that does make sense, and I think you're correct there. I couldn't really figure out why we actually need that increased timeout, something slows down the whole startup sequence, and we should probably clean that up instead of adjusting the timeout.
There was a problem hiding this comment.
@timcharper I've figured it out: The ConnectionPoolSettings creation does a reverse DNS lookup which is really slow on my macbook, which triggered the timeout. I've added a timeout check there and throw a reasonable error message now, so users of the lib will see something useful.
If we need timeouts they should be controlled by USI specific config, and not by changing Akka defaults. Doing the latter could lead to surprising behavior in frameworks that use Akka. Further, the override order for reference.conf files is non-deterministic. Depending on the packaging method used, one reference.conf file (from akka) could take precendence over another, and lead to non-deterministic config. Avoid.
| @@ -52,6 +54,28 @@ case class Session(url: URL, streamId: String, authorization: Option[Credentials | |||
| Flow[Array[Byte]].map(createPostRequest(_, None)).via(connection) | |||
| } | |||
|
|
|||
| private def poolSettings():Future[ConnectionPoolSettings] = Future { | |||
There was a problem hiding this comment.
What is the reason for this being a future?
| // dns lookup, which may take more than 5 seconds which triggers the akka streams subscription timeout. | ||
| // If we let the creation here take the full time, we won't start without any reasonable logging message | ||
| try { | ||
| Await.result(poolSettings(), 2.seconds) |
There was a problem hiding this comment.
I don't get this. I'm sorry. Why can we not document the fact that apps should increase the Akka stream subscription timeout? It feels we are just introducing complexity here.
|
This PR is still relevant, right? |
|
@DominikDary, yes. @timcharper or me should take this over. |
Fixed some timeout issues on startup
Fixed issues with PodIds containing .
Exit JVM if an error is thrown or the main loop exits
JIRA issues: DCOS_OSS-5633