typesafehub / akka-contrib-extra

ConductR Akka contributions
Other
9 stars 16 forks source link

Change reconnect API #23

Closed hseeberger closed 9 years ago

hseeberger commented 9 years ago

/cc @ktoso

ktoso commented 9 years ago

outgoingConnection should be renamed bindAndHandle

Disagree - bind... is used specifically for server-side, not the client-side.

Http.reconnect is too much magic => instead renamve ReconnectingStreamHttp to ReconnectingHttp

I thought you liked it during review ;-) No strong feelings on this one, can be removed. The ReconnectingHttp name sounds good, Tcp should be ReconnectingStreamTcp though, as the "normal one" is StreamTcp.

Do you want to do the changes?

hseeberger commented 9 years ago

Do you want to do the changes?

Not yet, I just wanted to start a discussion.

Indeed, bindAndHandle is no good, but as there's the "handler" parameter, the method is definitely providing more than just a connection. Maybe connectAndHandle?

ktoso commented 9 years ago

Passing in the handler flow is too limiting, it must be the onConnection callback style IMO (because you may need to get materialized values out for each materialization - e.g. my use case) (other way would be to make the return type a Source instead of a Future but that complicates things a bit... would rather not go there without a good reason).

Maybe something that points out that this may be multiple outgoing connections (for each reconnect)? How about outgoingConnections(...)(onConnection = ...)?

hseeberger commented 9 years ago

With handler I mean onConnect, were on the same page here. The issue I have with outgoingConnection (or the plural) is that it sounds too "less", the method does more that just return a connection, it applies the given onConnect handler. Maybe withOutgoingConnection?

ktoso commented 9 years ago

Removed reconnect utilities from contrail, they are not something we want to provide as such (at this level), see https://github.com/typesafehub/akka-contrib-extra/issues/30 and related tickets in akka for details.

ktoso commented 9 years ago

Can't close issues on this repo, please close this.