Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Propagate statusPromise down to the actual change in throttler, see #2877 #1002

Merged
merged 1 commit into from Jan 8, 2013

Conversation

Projects
None yet
5 participants
Owner

patriknw commented Jan 8, 2013

No description provided.

@drewhk drewhk commented on the diff Jan 8, 2013

akka-remote/src/main/scala/akka/remote/Endpoint.scala
@@ -1,3 +1,6 @@
+/**
+ * Copyright (C) 2009-2012 Typesafe Inc. <http://www.typesafe.com>
Member

drewhk commented Jan 8, 2013

Looks OK to me.

Owner

patriknw commented Jan 8, 2013

@drewhk could you take a look at the

when(WaitModeAndUpstreamListener) {
    case Event((listener: HandleEventListener, mode: ThrottleMode), _) ⇒

I didn't change that, but it is changing the inboundThrottleMode = mode

@viktorklang viktorklang and 2 others commented on an outdated diff Jan 8, 2013

...akka/remote/transport/ThrottlerTransportAdapter.scala
@@ -107,10 +111,8 @@ class ThrottlerTransportAdapter(_wrappedTransport: Transport, _system: ExtendedA
protected def managerProps = Props(new ThrottlerManager(wrappedTransport))
override def managementCommand(cmd: Any, statusPromise: Promise[Boolean]): Unit = cmd match {
- case s @ SetThrottle(_, _, _) ⇒
- manager ! s
- statusPromise.success(true)
- case _ ⇒ wrappedTransport.managementCommand(cmd, statusPromise)
+ case s @ SetThrottle(_, _, _) ⇒ manager ! ManagementCommand(s, statusPromise)
@viktorklang

viktorklang Jan 8, 2013

Owner

s: SetThrottle ?

@drewhk

drewhk Jan 8, 2013

Member

yes, that's nicer!

@patriknw

patriknw Jan 8, 2013

Owner

what do you mean?

@patriknw

patriknw Jan 8, 2013

Owner

ah, the params, of course

@viktorklang viktorklang and 1 other commented on an outdated diff Jan 8, 2013

...akka/remote/transport/ThrottlerTransportAdapter.scala
val naked = nakedAddress(address)
throttlingModes += naked -> (mode, direction)
handleTable.foreach {
case (addr, handle) ⇒
- if (addr == naked) setMode(handle, mode, direction)
+ if (addr == naked) setMode(handle, mode, direction, statusPromise)
}
@viktorklang

viktorklang Jan 8, 2013

Owner
case (`naked`, handle) ⇒ setMode(handle, mode, direction, statusPromise)
case _ ⇒
@patriknw

patriknw Jan 8, 2013

Owner

changed, thanks

@viktorklang viktorklang and 1 other commented on an outdated diff Jan 8, 2013

...akka/remote/transport/ThrottlerTransportAdapter.scala
inboundThrottleMode = mode
- if (inboundThrottleMode == Blackhole) {
+ try if (inboundThrottleMode == Blackhole) {
@viktorklang

viktorklang Jan 8, 2013

Owner

I'd use "mode" here, instead of reading a var

Owner

patriknw commented Jan 8, 2013

I fixed the review comments, and also the aggregated statusPromise at two places, see statusPromise completeWith Future.fold

Collaborator

akka-ci commented Jan 8, 2013

Started jenkins job akka-pr-validator at https://jenkins.akka.io/job/akka-pr-validator/283/

Collaborator

akka-ci commented Jan 8, 2013

jenkins job akka-pr-validator: Failed - https://jenkins.akka.io/job/akka-pr-validator/283/

Collaborator

rkuhn commented Jan 8, 2013

LGTM

Owner

patriknw commented Jan 8, 2013

alright, we consider the pr-validator failure (3 second timeout) as a temporary timing glitch for now, it will be seen again if it's a real problem

@patriknw patriknw added a commit that referenced this pull request Jan 8, 2013

@patriknw patriknw Merge pull request #1002 from akka/wip-2877-throttler-ack-patriknw
Propagate statusPromise down to the actual change in throttler, see #2877
5d53ec0

@patriknw patriknw merged commit 5d53ec0 into master Jan 8, 2013

@patriknw patriknw deleted the wip-2877-throttler-ack-patriknw branch Jan 8, 2013

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