Wip 2053 actorbased remote drewhk #652

Closed
wants to merge 54 commits into
from

Conversation

Projects
None yet
7 participants
@drewhk
Member

drewhk commented Aug 28, 2012

Do not merge! Just for review.

The important stuff is in ActorManagedRemoting. TransportConnector is the abstraction layer over the underlying transport medium. NettyConnector is a first-iteration implementation of the former, and DummyConnector is a Connector used for testing.

Beware: No docs yet, some tests are failing.

+ transport = loadTransport
+ headActor = system.asInstanceOf[ActorSystemImpl].systemActorOf(Props(new HeadActor(provider, transport, managedRemoteSettings)), HeadActorName)
+
+ val timeout = new Timeout(5 seconds)

This comment has been minimized.

@rkuhn

rkuhn Aug 28, 2012

Collaborator

these things tend to want to be configurable

@rkuhn

rkuhn Aug 28, 2012

Collaborator

these things tend to want to be configurable

+ private def retryLatchOpen(timeOfFailure: Long) = false
+
+ override def postStop() {
+ // TODO: All the children actors are stopped already?

This comment has been minimized.

@rkuhn

rkuhn Aug 28, 2012

Collaborator

yes, postStop of parent is called after all postStop of children have run

@rkuhn

rkuhn Aug 28, 2012

Collaborator

yes, postStop of parent is called after all postStop of children have run

This comment has been minimized.

@drewhk

drewhk Aug 28, 2012

Member

After all the postStop of the children have been called, or after all the postStop finished?

@drewhk

drewhk Aug 28, 2012

Member

After all the postStop of the children have been called, or after all the postStop finished?

This comment has been minimized.

@bantonsson

bantonsson Aug 28, 2012

Member

The child tells the parent that it has terminated after it has run its postStop. And the parent proceeds to run its own postStop after all children has told it that they have terminated.

Does that answer your question?

@bantonsson

bantonsson Aug 28, 2012

Member

The child tells the parent that it has terminated after it has run its postStop. And the parent proceeds to run its own postStop after all children has told it that they have terminated.

Does that answer your question?

+ private var transport: RemoteTransport = _
+
+ override val supervisorStrategy = OneForOneStrategy(maxNrOfRetries = 10, withinTimeRange = 1 minute) {
+ case _: EndpointException Stop

This comment has been minimized.

@rkuhn

rkuhn Aug 28, 2012

Collaborator

This means that all other exceptions are escalated, leading to a restart of the headActor, which will due to default preRestart terminate all connections. I know you are thinking about definitions of restart semantics for the connection actors, and once that is done it needs to be hooked in here.

@rkuhn

rkuhn Aug 28, 2012

Collaborator

This means that all other exceptions are escalated, leading to a restart of the headActor, which will due to default preRestart terminate all connections. I know you are thinking about definitions of restart semantics for the connection actors, and once that is done it needs to be hooked in here.

+ def logicalLinkToNetworkLink(link: (Address, Address)): (HostAndPort, HostAndPort) = addressToHostAndPort(link._1) -> addressToHostAndPort(link._2)
+
+ def silentDrop(source: Address) {
+ droppingSet += source

This comment has been minimized.

@rkuhn

rkuhn Aug 28, 2012

Collaborator

adding @volatile does not make it race-free without using CAS loops; I'd recommend a normal ConcurrentHashMap or similar

@rkuhn

rkuhn Aug 28, 2012

Collaborator

adding @volatile does not make it race-free without using CAS loops; I'd recommend a normal ConcurrentHashMap or similar

This comment has been minimized.

@drewhk

drewhk Aug 28, 2012

Member

Totally true, I realized that it was wrong, but I forgot about concurrent collections.

@drewhk

drewhk Aug 28, 2012

Member

Totally true, I realized that it was wrong, but I forgot about concurrent collections.

+ * User: Varga Endre Sándor
+ * Date: 2012.08.20.
+ * Time: 14:58
+ * To change this template use File | Settings | File Templates.

This comment has been minimized.

@rkuhn

rkuhn Aug 28, 2012

Collaborator

probably the easiest way of not forgetting to remove these is to switch them off in IDEA ;-)

@rkuhn

rkuhn Aug 28, 2012

Collaborator

probably the easiest way of not forgetting to remove these is to switch them off in IDEA ;-)

+
+abstract class TransportConnector(val system: ExtendedActorSystem, val provider: RemoteActorRefProvider) {
+
+ import akka.actor.Address

This comment has been minimized.

@rkuhn

rkuhn Aug 28, 2012

Collaborator

we usually put such imports at the top-level, unless lexical scope really matters (for implicits) or they are "unusual"

@rkuhn

rkuhn Aug 28, 2012

Collaborator

we usually put such imports at the top-level, unless lexical scope really matters (for implicits) or they are "unusual"

+ import akka.actor.Address
+
+ def responsibleActor: ActorRef
+ def responsibleActor_=(actor: ActorRef): Unit

This comment has been minimized.

@rkuhn

rkuhn Aug 28, 2012

Collaborator

keep an eye out for Java API: we generally do not use (partly) symbolic method names on interfaces which can reasonably be implemented by user code

@rkuhn

rkuhn Aug 28, 2012

Collaborator

keep an eye out for Java API: we generally do not use (partly) symbolic method names on interfaces which can reasonably be implemented by user code

+ import akka.remote.RemoteActorRef
+
+ def responsibleActor: ActorRef
+ def responsibleActor_=(actor: ActorRef): Unit

This comment has been minimized.

@rkuhn

rkuhn Aug 28, 2012

Collaborator

didn't look to closely at the usage: why does the handle need to be mutable?

@rkuhn

rkuhn Aug 28, 2012

Collaborator

didn't look to closely at the usage: why does the handle need to be mutable?

+ }
+
+ "connect to remote address at the first send" in withCleanTransport {
+ remoteReference ! "discard"

This comment has been minimized.

@rkuhn

rkuhn Aug 28, 2012

Collaborator

maybe assert that no connection was there before the send

@rkuhn

rkuhn Aug 28, 2012

Collaborator

maybe assert that no connection was there before the send

+ }
+
+ override def connect(remote: Address, responsibleActorForConnection: ActorRef) {
+ if (_isTerminated) throw new IllegalStateException("Cannot connect: already terminated")

This comment has been minimized.

@viktorklang

viktorklang Sep 5, 2012

Member

_isTerminated accessed outside of lock and isn't volatile. Also, name is confusing, perhaps something like _shutdownRequested

@viktorklang

viktorklang Sep 5, 2012

Member

_isTerminated accessed outside of lock and isn't volatile. Also, name is confusing, perhaps something like _shutdownRequested

This comment has been minimized.

@drewhk

drewhk Sep 5, 2012

Member

Yes, these classes related to DummyTransport are not threadsafe. This is
something I intend to clean up, but only after the design of the remoting
is settled.

@drewhk

drewhk Sep 5, 2012

Member

Yes, these classes related to DummyTransport are not threadsafe. This is
something I intend to clean up, but only after the design of the remoting
is settled.

+
+ // Instead of using different sets, use a map of options and patter matching instead of ifs
+ if (dummyMedium.shouldCrash(address)) {
+ throw new NullPointerException

This comment has been minimized.

@viktorklang

viktorklang Sep 5, 2012

Member

?

This comment has been minimized.

@drewhk

drewhk Sep 5, 2012

Member

This is only used for testing the EndpointActor's behavior during
restarts. This code is used to intentionally crash the actor and force a
restart.

@drewhk

drewhk Sep 5, 2012

Member

This is only used for testing the EndpointActor's behavior during
restarts. This code is used to intentionally crash the actor and force a
restart.

+ responsibleActorForConnection ! ConnectionFailed(new IllegalStateException("Rejected"))
+ } else if (!dummyMedium.shouldDrop(address)) {
+ dummyMedium.registerConnection(address -> remote, this, responsibleActorForConnection) match {
+ case Some((localHandle, remoteHandle)) {

This comment has been minimized.

@viktorklang

viktorklang Sep 5, 2012

Member

braces not needed

@viktorklang

viktorklang Sep 5, 2012

Member

braces not needed

+ } else if (!dummyMedium.shouldDrop(address)) {
+ dummyMedium.registerConnection(address -> remote, this, responsibleActorForConnection) match {
+ case Some((localHandle, remoteHandle)) {
+ handles ::= localHandle

This comment has been minimized.

@viktorklang

viktorklang Sep 5, 2012

Member

A Sequence might not be a proper data structure for this, a Set perhaps?

@viktorklang

viktorklang Sep 5, 2012

Member

A Sequence might not be a proper data structure for this, a Set perhaps?

+ handles ::= localHandle
+ responsibleActorForConnection ! ConnectionInitialized(localHandle)
+ }
+ case None responsibleActorForConnection ! ConnectionFailed(new IllegalArgumentException("Remote address does not reachable"))

This comment has been minimized.

@viktorklang

viktorklang Sep 5, 2012

Member

I'm skipping the rest of the Dummy* as I assume this code will be dropped later in the process.

@viktorklang

viktorklang Sep 5, 2012

Member

I'm skipping the rest of the Dummy* as I assume this code will be dropped later in the process.

+import akka.serialization.Serialization
+import akka.remote.MessageSerializer
+
+trait MessageEncodings {

This comment has been minimized.

@viktorklang

viktorklang Sep 5, 2012

Member

I propose "RemoteMessageEncoder"

@viktorklang

viktorklang Sep 5, 2012

Member

I propose "RemoteMessageEncoder"

+import actmote.TransportConnector.ConnectionInitialized
+import actmote.TransportConnector.ConnectorFailed
+import actmote.TransportConnector.ConnectorInitialized
+import actmote.TransportConnector.IncomingConnection

This comment has been minimized.

@viktorklang

viktorklang Sep 5, 2012

Member

All of these imports are superfluous (after ._)

@viktorklang

viktorklang Sep 5, 2012

Member

All of these imports are superfluous (after ._)

+import org.jboss.netty.util._
+import org.jboss.netty.channel._
+import akka.event.Logging
+import group._

This comment has been minimized.

@viktorklang

viktorklang Sep 5, 2012

Member

We never use relative imports, they are confusing and order-dependent

@viktorklang

viktorklang Sep 5, 2012

Member

We never use relative imports, they are confusing and order-dependent

This comment has been minimized.

@drewhk

drewhk Sep 5, 2012

Member

We never use relative imports, they are confusing and order-dependent

This is "IDEA magic", I have to turn this thing off... Thanks for noticing!

@drewhk

drewhk Sep 5, 2012

Member

We never use relative imports, they are confusing and order-dependent

This is "IDEA magic", I have to turn this thing off... Thanks for noticing!

+import scala.Some
+import akka.actor.DeadLetter
+import org.jboss.netty.handler.ssl.SslHandler
+import scala.Some

This comment has been minimized.

@viktorklang

viktorklang Sep 5, 2012

Member

scala._ is always imported by default, and here scala.Some is imported explicitly, twice

@viktorklang

viktorklang Sep 5, 2012

Member

scala._ is always imported by default, and here scala.Some is imported explicitly, twice

+ /**
+ * Backing scaffolding for the default implementation of NettyRemoteSupport.createPipeline.
+ */
+ object PipelineFactory {

This comment has been minimized.

@viktorklang

viktorklang Sep 5, 2012

Member

object-in-class why?

@viktorklang

viktorklang Sep 5, 2012

Member

object-in-class why?

This comment has been minimized.

@drewhk

drewhk Sep 6, 2012

Member

I copied this from NettyRemoteTransport. I should have asked first why is
it an object... I will fix it. Should the original code
(NettyRemoteTransport) stay as it is?

@drewhk

drewhk Sep 6, 2012

Member

I copied this from NettyRemoteTransport. I should have asked first why is
it an object... I will fix it. Should the original code
(NettyRemoteTransport) stay as it is?

This comment has been minimized.

@viktorklang

viktorklang Sep 6, 2012

Member

It's not the fact that it is an object that is a problem, but why is it an object within a class?

@viktorklang

viktorklang Sep 6, 2012

Member

It's not the fact that it is an object that is a problem, but why is it an object within a class?

This comment has been minimized.

@drewhk

drewhk Sep 6, 2012

Member

Because that's how it is in NettyRemoteTransport (it is a class as well)
and I thought there is a reason for that which I just don't understand.
Check NettyRemoteSupport.scala line 345. I am not saying it is right :) I
will fix it anyway, but should I fix it in NettyRemoteTransport?

@drewhk

drewhk Sep 6, 2012

Member

Because that's how it is in NettyRemoteTransport (it is a class as well)
and I thought there is a reason for that which I just don't understand.
Check NettyRemoteSupport.scala line 345. I am not saying it is right :) I
will fix it anyway, but should I fix it in NettyRemoteTransport?

This comment has been minimized.

@viktorklang

viktorklang Sep 6, 2012

Member

If it was like that already then just leave it be, I just got curious :-)

@viktorklang

viktorklang Sep 6, 2012

Member

If it was like that already then just leave it be, I just got curious :-)

This comment has been minimized.

@drewhk

drewhk Sep 6, 2012

Member

Ok, but I should change it in my code (NettyConnector), right?

@drewhk

drewhk Sep 6, 2012

Member

Ok, but I should change it in my code (NettyConnector), right?

This comment has been minimized.

@viktorklang

viktorklang Sep 6, 2012

Member

Best solution is probably to pull it out and use the same version from both the NettyConnector and the NettyRemoteTransport

@viktorklang

viktorklang Sep 6, 2012

Member

Best solution is probably to pull it out and use the same version from both the NettyConnector and the NettyRemoteTransport

+import akka.actor.DeadLetter
+
+private[akka] object ChannelHandle extends ChannelLocal[NettyConnectorHandle] {
+ override def initialValue(channel: Channel) = null

This comment has been minimized.

@viktorklang

viktorklang Sep 6, 2012

Member

Isn't the initial value always null?

@viktorklang

viktorklang Sep 6, 2012

Member

Isn't the initial value always null?

This comment has been minimized.

@drewhk

drewhk Sep 6, 2012

Member

True. It was an Option before, and I changed back to using null.

@drewhk

drewhk Sep 6, 2012

Member

True. It was an Option before, and I changed back to using null.

@@ -0,0 +1,487 @@
+package akka.remote.actmote

This comment has been minimized.

@patriknw

patriknw Sep 6, 2012

Member

I guess actmote is just a temporary code name?

@patriknw

patriknw Sep 6, 2012

Member

I guess actmote is just a temporary code name?

This comment has been minimized.

@drewhk

drewhk Sep 6, 2012

Member

I think you can consider all names as temporary. Any naming suggestions
are welcome.

@drewhk

drewhk Sep 6, 2012

Member

I think you can consider all names as temporary. Any naming suggestions
are welcome.

+
+ def shutdown() {
+ if (headActor != null) {
+ try {

This comment has been minimized.

@patriknw

patriknw Sep 6, 2012

Member
if (headActor != null) try {
@patriknw

patriknw Sep 6, 2012

Member
if (headActor != null) try {
+ def shutdown() {
+ if (headActor != null) {
+ try {
+ val stopped: Future[Boolean] = gracefulStop(headActor, 5 seconds)(system)

This comment has been minimized.

@patriknw

patriknw Sep 6, 2012

Member

magic value: 5 seconds
same as managedRemoteSettings.ShutdownTimeout?

@patriknw

patriknw Sep 6, 2012

Member

magic value: 5 seconds
same as managedRemoteSettings.ShutdownTimeout?

+ connector = loadConnector
+ // NOTE: Notifier will use the logger of this class
+ val notifier = new DefaultLifeCycleNotifier(provider.remoteSettings, this, system.eventStream, log)
+ headActor = system.asInstanceOf[ActorSystemImpl].systemActorOf(Props(new HeadActor(provider.remoteSettings, connector, managedRemoteSettings, notifier)), HeadActorName)

This comment has been minimized.

@patriknw

patriknw Sep 6, 2012

Member

what if multiple threads do start at the same time?
do we need to support that?

@patriknw

patriknw Sep 6, 2012

Member

what if multiple threads do start at the same time?
do we need to support that?

This comment has been minimized.

@drewhk

drewhk Sep 6, 2012

Member

I don't know. I will check it. I think you can currently specify one
RemoteTransport in the config and that is started by
RemoteActorRefProvider.

@drewhk

drewhk Sep 6, 2012

Member

I don't know. I will check it. I think you can currently specify one
RemoteTransport in the config and that is started by
RemoteActorRefProvider.

+ val notifier = new DefaultLifeCycleNotifier(provider.remoteSettings, this, system.eventStream, log)
+ headActor = system.asInstanceOf[ActorSystemImpl].systemActorOf(Props(new HeadActor(provider.remoteSettings, connector, managedRemoteSettings, notifier)), HeadActorName)
+
+ val timeout = new Timeout(managedRemoteSettings.StartupTimeout)

This comment has been minimized.

@patriknw

patriknw Sep 6, 2012

Member

use apply of the case class Timeout instead of new

val timeout = Timeout(
@patriknw

patriknw Sep 6, 2012

Member

use apply of the case class Timeout instead of new

val timeout = Timeout(
+ def remoteServerShutdown() { notifyListeners(RemoteServerShutdown(remoteTransport)) }
+ def remoteServerError(reason: Throwable) { notifyListeners(RemoteServerError(reason, remoteTransport)) }
+ def remoteServerClientConnected(remoteAddress: Address) { notifyListeners(RemoteServerClientConnected(remoteTransport, Some(remoteAddress))) }
+ def remoteServerClientDisconnected(remoteAddress: Address) { notifyListeners(RemoteServerClientDisconnected(remoteTransport, Some(remoteAddress))) }

This comment has been minimized.

@patriknw

patriknw Sep 6, 2012

Member

I think we have a convention to use max 120 chars lines.
Not followed everywhere though.

@patriknw

patriknw Sep 6, 2012

Member

I think we have a convention to use max 120 chars lines.
Not followed everywhere though.

+
+ def getEndpointWithPolicy(address: Address) = addressToEndpointAndPolicy.get(address)
+
+ def prune(pruneAge: Duration) {

This comment has been minimized.

@patriknw

patriknw Sep 6, 2012

Member

: Unit = {

@patriknw

patriknw Sep 6, 2012

Member

: Unit = {

+
+ override def postRestart(reason: Throwable) {
+ // Clear handle to force reconnect
+ handleOption = None

This comment has been minimized.

@patriknw

patriknw Sep 6, 2012

Member

so, after a restart you don't want to use the handleOption passed in to the constructor, but still use isServer derived from the constructor param?

@patriknw

patriknw Sep 6, 2012

Member

so, after a restart you don't want to use the handleOption passed in to the constructor, but still use isServer derived from the constructor param?

This comment has been minimized.

@drewhk

drewhk Sep 6, 2012

Member

Yikes. Good catch.

@drewhk

drewhk Sep 6, 2012

Member

Yikes. Good catch.

+ }
+
+ override def unhandled(message: Any) {
+ throw new EndpointConnectorProtocolViolated(remoteAddress, "Endpoint <-> Connector protocol violated; unexpected message: " + message)

This comment has been minimized.

@patriknw

patriknw Sep 6, 2012

Member

perhaps call super.unhandled first?

@patriknw

patriknw Sep 6, 2012

Member

perhaps call super.unhandled first?

+ remoteMessage.payload match {
+ case m @ (_: DaemonMsg | _: Terminated)
+ try remoteDaemon ! m catch {
+ case e: Exception log.error(e, "exception while processing remote command {} from {}", m, remoteMessage.sender)

This comment has been minimized.

@patriknw

patriknw Sep 6, 2012

Member

do we need catch around !
in that case it should perhaps be case NonFatal(e)

@patriknw

patriknw Sep 6, 2012

Member

do we need catch around !
in that case it should perhaps be case NonFatal(e)

This comment has been minimized.

@viktorklang

viktorklang Sep 6, 2012

Member

no, ! should not throw exceptions in 2.1

@viktorklang

viktorklang Sep 6, 2012

Member

no, ! should not throw exceptions in 2.1

This comment has been minimized.

@drewhk

drewhk Sep 6, 2012

Member

This is copied from the old code. Should I fix it there as well?

@drewhk

drewhk Sep 6, 2012

Member

This is copied from the old code. Should I fix it there as well?

This comment has been minimized.

@viktorklang

viktorklang Sep 6, 2012

Member

Sure thanks!

@viktorklang

viktorklang Sep 6, 2012

Member

Sure thanks!

+ try remoteDaemon ! m catch {
+ case e: Exception log.error(e, "exception while processing remote command {} from {}", m, remoteMessage.sender)
+ }
+ case x log.warning("remoteDaemon received illegal message {} from {}", x, remoteMessage.sender)

This comment has been minimized.

@patriknw

patriknw Sep 6, 2012

Member

rely on unhandled instead?

@patriknw

patriknw Sep 6, 2012

Member

rely on unhandled instead?

This comment has been minimized.

@patriknw

patriknw Sep 6, 2012

Member

sorry, this was not an actor

@patriknw

patriknw Sep 6, 2012

Member

sorry, this was not an actor

+ */
+ sealed trait ConnectorEvent
+
+ /**

This comment has been minimized.

@patriknw

patriknw Sep 6, 2012

Member

awesome documentation in here!

@patriknw

patriknw Sep 6, 2012

Member

awesome documentation in here!

+ if (!channel.isWritable) {
+ false // Signal backoff
+ } else {
+ val f = channel.write(request)

This comment has been minimized.

@patriknw

patriknw Sep 6, 2012

Member

val f not needed

@patriknw

patriknw Sep 6, 2012

Member

val f not needed

+ //throw new RemoteClientException("Unknown message received in remoteAddress client handler: " + other, client.netty, client.remoteAddress)
+ }
+ } catch {
+ case e: Exception //TODO: notify endpoint of error

This comment has been minimized.

@patriknw

patriknw Sep 6, 2012

Member

NonFatal?
or is there a reason for e: Exception?

@patriknw

patriknw Sep 6, 2012

Member

NonFatal?
or is there a reason for e: Exception?

+ private val openChannels: ChannelGroup = new DefaultDisposableChannelGroup("akka-remote-server")
+ @volatile private var channel: Channel = _
+
+ val ip = InetAddress.getByName(settings.Hostname)

This comment has been minimized.

@viktorklang

viktorklang Sep 6, 2012

Member

Should probably be documented that this might take a while

@viktorklang

viktorklang Sep 6, 2012

Member

Should probably be documented that this might take a while

This comment has been minimized.

@drewhk

drewhk Sep 6, 2012

Member

Ok!

@patriknw

This comment has been minimized.

Show comment
Hide comment
@patriknw

patriknw Sep 6, 2012

Member

Very good job!

Member

patriknw commented Sep 6, 2012

Very good job!

+
+ def connect(remoteAddress: Address, responsibleActorForConnection: ActorRef) {
+ val name = Logging.simpleName(this) + "@" + remoteAddress
+ val remoteIP = InetAddress.getByName(remoteAddress.host.get)

This comment has been minimized.

@viktorklang

viktorklang Sep 6, 2012

Member

This could also get problematic.

@viktorklang

viktorklang Sep 6, 2012

Member

This could also get problematic.

This comment has been minimized.

@drewhk

drewhk Sep 6, 2012

Member

Yes, this is blocking -- I just realized it yesterday. I could not find a
non-blocking name lookup in Java. Can you recommend a solution?

@drewhk

drewhk Sep 6, 2012

Member

Yes, this is blocking -- I just realized it yesterday. I could not find a
non-blocking name lookup in Java. Can you recommend a solution?

+ if (future.isPartialFailure) {
+ responsibleActor ! ConnectorFailed(new CleanShutdownFailedException)
+ }
+ serverBootstrap.releaseExternalResources()

This comment has been minimized.

@viktorklang

viktorklang Sep 6, 2012

Member

is this pasted here verbatim?

@viktorklang

viktorklang Sep 6, 2012

Member

is this pasted here verbatim?

This comment has been minimized.

@drewhk

drewhk Sep 6, 2012

Member

Partly.

@drewhk

drewhk Sep 6, 2012

Member

Partly.

This comment has been minimized.

@viktorklang

viktorklang Sep 6, 2012

Member

WDYM?

This comment has been minimized.

@drewhk

drewhk Sep 6, 2012

Member

This code was lifted from the original transport, but I started to rewrite
it because of the blocking calls (awaitUninterruptibly) but I have not
finished rewriting it in a completely asynchronous fashion.

@drewhk

drewhk Sep 6, 2012

Member

This code was lifted from the original transport, but I started to rewrite
it because of the blocking calls (awaitUninterruptibly) but I have not
finished rewriting it in a completely asynchronous fashion.

+ val openChannels: ChannelGroup,
+ val connector: NettyConnector) extends SimpleChannelUpstreamHandler {
+
+ val log = connector.log

This comment has been minimized.

@viktorklang

viktorklang Sep 6, 2012

Member

why?

+
+ override def channelDisconnected(ctx: ChannelHandlerContext, event: ChannelStateEvent) = {
+ log.info("Inbound TCP connection closed from {}", event.getChannel.getRemoteAddress)
+ val handle: NettyConnectorHandle = ChannelHandle.get(event.getChannel)

This comment has been minimized.

@viktorklang

viktorklang Sep 6, 2012

Member

Why explicit type here?

@viktorklang

viktorklang Sep 6, 2012

Member

Why explicit type here?

+ override def messageReceived(ctx: ChannelHandlerContext, event: MessageEvent) = try {
+ // TODO: disconnect on protocol error
+ event.getMessage match {
+ case remote: AkkaRemoteProtocol if remote.hasMessage {

This comment has been minimized.

@viktorklang

viktorklang Sep 6, 2012

Member

no need for braces

@viktorklang

viktorklang Sep 6, 2012

Member

no need for braces

+ channel.close()
+ }
+
+ override def write(msg: Any, senderOption: Option[ActorRef], recipient: RemoteActorRef) = {

This comment has been minimized.

@viktorklang

viktorklang Sep 6, 2012

Member

I think the write-method should return a Future so that the logic inside the method could be extracted out, because all other transportconnectors will want to achieve the same behavior (sending to DeadLetters etc).

@viktorklang

viktorklang Sep 6, 2012

Member

I think the write-method should return a Future so that the logic inside the method could be extracted out, because all other transportconnectors will want to achieve the same behavior (sending to DeadLetters etc).

This comment has been minimized.

@drewhk

drewhk Sep 6, 2012

Member

You mean that the handling of errors should move to the EndpointActor (do
the same stuff as above when the Future fails)? I was actually thinking
about that and talked with Roland about it. He told me that as we already
have an ActorRef, a Future might be overkill. I am personally a bit
undecided about this. I conceptionally like the Future version better, but
on the other hand I feel it is a bit heavyweight. Does it have any
performance implications? What is your opinion?

@drewhk

drewhk Sep 6, 2012

Member

You mean that the handling of errors should move to the EndpointActor (do
the same stuff as above when the Future fails)? I was actually thinking
about that and talked with Roland about it. He told me that as we already
have an ActorRef, a Future might be overkill. I am personally a bit
undecided about this. I conceptionally like the Future version better, but
on the other hand I feel it is a bit heavyweight. Does it have any
performance implications? What is your opinion?

+ true
+ }
+ } catch {
+ case NonFatal(e) {

This comment has been minimized.

@viktorklang

viktorklang Sep 6, 2012

Member

No need for braces (check all such occurences)

@viktorklang

viktorklang Sep 6, 2012

Member

No need for braces (check all such occurences)

+ * - must be only sent to the responsible actor of the connector
+ * @param reason the cause of the failure
+ */
+ case class ConnectorFailed(reason: Throwable)

This comment has been minimized.

@viktorklang

viktorklang Sep 6, 2012

Member

doesn't extend ConnectorEvent, on purpose?

@viktorklang

viktorklang Sep 6, 2012

Member

doesn't extend ConnectorEvent, on purpose?

This comment has been minimized.

@drewhk

drewhk Sep 6, 2012

Member

No. It's a bug... Sigh

@drewhk

drewhk Sep 6, 2012

Member

No. It's a bug... Sigh

+ startup-timeout = 5 s
+ shutdown-timeout = 5 s
+ preconnect-buffer-size = 1000
+ retry-latch-closed-for = 0

This comment has been minimized.

@viktorklang

viktorklang Sep 6, 2012

Member

0 ms?

This comment has been minimized.

@drewhk

drewhk Sep 6, 2012

Member

Do zeros need units?

@drewhk

drewhk Sep 6, 2012

Member

Do zeros need units?

This comment has been minimized.

@viktorklang

viktorklang Sep 6, 2012

Member

No, but for the casual reader or the guy who works with this test, it is nocer to know instantly if its a count or a duration.

@viktorklang

viktorklang Sep 6, 2012

Member

No, but for the casual reader or the guy who works with this test, it is nocer to know instantly if its a count or a duration.

@drewhk

This comment has been minimized.

Show comment
Hide comment
@drewhk

drewhk Sep 7, 2012

Member

This Pull request became obsolete by the new design. Closing.

Member

drewhk commented Sep 7, 2012

This Pull request became obsolete by the new design. Closing.

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