betfair / cougar

Cougar is a framework for making building network exposed service interfaces easy.
http://betfair.github.io/cougar
Apache License 2.0
27 stars 18 forks source link

QoS - Client specified expiry #55

Closed eswdd closed 9 years ago

eswdd commented 10 years ago

From #19:

Client timestamps in calls, EV answers with suitable code if max-age/expiry time is reached (effectively drops internal requests that have sat in the surge queue or the send buf (bin)) for too long Allow clients to specify an inbound operation time that the client would specify – that way, we don’t bother to accept a call if it has already aged while it has been in the surge queue

eswdd commented 10 years ago

Work on this is ongoing in the client-timeout branch. Currently an expiryTime is passed throughout (so no longer just on the sync client interface). However this doesn't take into account time sync issues.

Thinking about changing the parameter to TimingConstraints object, which contains the original client parameter (timeoutMillis) plus a resolved expiryTime, which is based on the time synchronicity between client and server (we'll need some way to resolve this based on the request). If the time synchronicity is unknown then we have to use the timeoutMillis, if it's perfect, then we can use the client resolved expiry time, if we don't know, then what?

@richardqd any thoughts?

richardqd commented 10 years ago

I understand where you're coming from with this, but I wonder if it isn't over complicating it?

If it is going to be used as a blunt 'save our bacon' instrument, then the clock sync isn't terribly important. --Who cares if there's 23ms of drift - if our timeout is 2s ....

If it is going to be used in extremely fine grained fashion, could we just suggest anybody who is using it run NTP, and be done with it?

bfmike commented 10 years ago

It shouldn't matter if there's 5 minutes of clock sync. Even with NTP that happens from time to time and you wouldn't want to reject perfectly fine requests because of large clock drift. If I was implementing this I would add a timestamp for when the request was received on the socket and keep the timestamp and timeout distinct data elements. Then if you need to call an underlying service you can work out how long has been spend and remove that from the timeoutMillis parameter you pass onwards.

eswdd commented 10 years ago

@richardqd - I'm rather thinking about calls from external clients where they don't necessarily sync with the same NTP source as us. Does everyone globally go down to the same atomic clock?

@bfmike - i'm exactly thinking of holding the data items seperately, but it does complicate the code somewhat

eswdd commented 10 years ago

So originally i was thinking of just sending across the max time, but this doesn't help if a request is stuck in a load balancer surge queue, which means a client needs to send an absolute time. Now if we know that the 2 hosts are perfectly in sync (or near enough - ie on the same LAN) then we can use the absolute time. If we don't know the state of their time syncronisation then we can't rely on this value and so we end up with the approach @bfmike talks about.

richardqd commented 10 years ago

I think it is an important point - that one of the things we were hoping for is to remove things that have been ageing in the surge queue, and unless we could code a module into the Surge queue, (which is possible), I don't see that we can get any benefit.

How does NTP deal with the lag of communicating a time?

On 27 November 2013 13:02, Simon Matic Langford notifications@github.comwrote:

So originally i was thinking of just sending across the max time, but this doesn't help if a request is stuck in a load balancer surge queue, which means a client needs to send an absolute time. Now if we know that the 2 hosts are perfectly in sync (or near enough - ie on the same LAN) then we can use the absolute time. If we don't know the state of their time syncronisation then we can't rely on this value and so we end up with the approach @bfmike https://github.com/bfmike talks about.

— Reply to this email directly or view it on GitHubhttps://github.com/betfair/cougar/issues/55#issuecomment-29382569 .

eswdd commented 10 years ago

Don't understand that q..

eswdd commented 10 years ago

The QoS interceptor framework I built for #19 gives us a hook before and after the EV queue, specifically for dealing with items which have aged in the surge queue - all we need to do now is identify the buggers.

richardqd commented 10 years ago

My understanding of Mike's suggestion is that we write a time down upon first receipt.

We will lose one of the things we were hoping to gain from it, since the age while it was inbound in the surge queue won't be measured.

This is in fact the reason for the requirement .... to ensure we're not processing things that have been in a surge queue for 30s. obv an ERO request has no meaning anyway, so there's little point in responding.

So this lead to - could we publish our time to the environment service to clients? We immediately run into a time of distribution on latency varying networks of course, and this is where the NTP comes from, since they've clearly solved it

On 27 November 2013 14:35, Simon Matic Langford notifications@github.comwrote:

The QoS interceptor framework I built for #19https://github.com/betfair/cougar/issues/19gives us a hook before and after the EV queue, specifically for dealing with items which have aged in the surge queue - all we need to do now is identify the buggers.

— Reply to this email directly or view it on GitHubhttps://github.com/betfair/cougar/issues/55#issuecomment-29388413 .

eswdd commented 10 years ago

So my intent was pretty much to rely on NTP so we don't reimplement - namely if the client passes something across to the server to indicate they think their time is syncronised with the servers (possibly we have something on the server which allows it to resolve time synchronicity, so within BF all internal requests are assumed to be synchronised for example), then we'll treat the time as absolute. so if we know someone's time is in sync, we can trust the time they send and get the gain from the surge queue for internal clients.

We used to expose time on the internal env service, but it's got other stuff on it which is sensitive so it's no longer exposed externally. The NTP algo for dealing with nw latency is pretty simple, but do we really want to be reimplementing that? Couldn't we just expose an NTP sync server to browser clients, make our webpage send the info, make aping send the info and encourage bot writers to sync with that source?

richardqd commented 10 years ago

Agreed - this would be the simplest approach.

I don't think we want to be reimplementing NTP, it was just that I wondered how this problem has been solved in the past.

Rich

On 27 November 2013 16:56, Simon Matic Langford notifications@github.comwrote:

So my intent was pretty much to rely on NTP so we don't reimplement - namely if the client passes something across to the server to indicate they think their time is syncronised with the servers (possibly we have something on the server which allows it to resolve time synchronicity, so within BF all internal requests are assumed to be synchronised for example), then we'll treat the time as absolute. so if we know someone's time is in sync, we can trust the time they send and get the gain from the surge queue for internal clients.

We used to expose time on the internal env service, but it's got other stuff on it which is sensitive so it's no longer exposed externally. The NTP algo for dealing with nw latency is pretty simple, but do we really want to be reimplementing that? Couldn't we just expose an NTP sync server to browser clients, make our webpage send the info, make aping send the info and encourage bot writers to sync with that source?

— Reply to this email directly or view it on GitHubhttps://github.com/betfair/cougar/issues/55#issuecomment-29400499 .

eswdd commented 10 years ago

LMGTFY :P

They basically work out the RTT by diffing the time it takes to process on the server from that on the client and then use some statistical stuff to push the time so that it differs by the RTT/2

jamiei commented 10 years ago

@eswdd RE External clients, I'm sure clients with bespoke operations would be happy to do this. For external clients who don't pass in the "I'm NTP synchronised" flag, presumably we would still allow the timeoutMillis equivalent to be passed in, with the provisos mentioned?

eswdd commented 10 years ago

Yes that's what I was thinking.

atropineal commented 10 years ago

That's what I would have thought - have an NTP server visible to the outside world and tell people they can sync with that if they want. No wheel reinvention.

On Wed, Nov 27, 2013 at 11:11 PM, Simon Matic Langford < notifications@github.com> wrote:

Yes that's what I was thinking.

— Reply to this email directly or view it on GitHubhttps://github.com/betfair/cougar/issues/55#issuecomment-29420418 .

atropineal commented 10 years ago

At the risk of being interested in something that's none of my business, what's this "I'm NTP synchronised" flag? Don't clients just pass their time in, and if it's NTP synchronized or not then we use it?

eswdd commented 10 years ago

The problem is around the client specifying a timeout. You have 2 choices:

What we're suggesting is that if we can identify that they're synchronised (specifically with a time server close to us that we're also synchronised with - as opposed to both of us eventually being synchronised by syncing to the strata 0 atomic clocks), then we can safely take approach 1, giving us the added feature, and if we can't then we play it safe and go approach 2.

richardqd commented 10 years ago

Another possibility I think is that perhaps one's loadbalancer could write an inbound time at the ingress point. The client could only specify a max TTL. We can then resolve that and run with it through Cougar..?

On 27 November 2013 21:49, Simon Matic Langford notifications@github.comwrote:

The problem is around the client specifying a timeout. You have 2 choices:

  • Take a literal/absolute expiry time from them in which case lack of clock sync might lead you to reject earlier than intended.
  • Take a max elapsed time from them in which case we can't reject requests that have aged in some intermediate device such as load balancer

What we're suggesting is that if we can identify that they're synchronised (specifically with a time server close to us that we're also synchronised with - as opposed to both of us eventually being synchronised by syncing to the strata 0 atomic clocks), then we can safely take approach 1, giving us the added feature, and if we can't then we play it safe and go approach 2.

— Reply to this email directly or view it on GitHubhttps://github.com/betfair/cougar/issues/55#issuecomment-29423053 .

atropineal commented 10 years ago

Why do you want clients to be specifying a timeout anyway? Why shouldn't the server just dump all requests that are older than so-many-millis or whatever? Perhaps discriminating by type of message. For example operation O1 has contract that says it will be dumped if over T1 millis at processing time, operation O2 will be dumped if over T2 millis... (could even say that O1 will be dumped after T1 if the request has such-and-such characteristics, and T2 if other characteristics).

I wonder whether you want a client telling you what to do. Might just be easier all around if the server tells everybody what it WILL do?

Bear in mind I don't know anything about who raised this requirement, why, or how much actual real life benefit it will bring to anybody. Is it just somebody's pet brainchild that sounds nice to have, or does it solve a very real problem?

richardqd commented 10 years ago

A very real problem we experienced during a recent bingle was that queued requests were held in a 50k+ item netscaler queue. This isn't good because consider any time sensitive requests in that queue, by the time the answer was calculated, it was irrelevant to the caller anyway. Worse, the service under the cosh had to go on and service those 50k+ requests anyway, even though the results will almost certainly have been discarded by the caller. Ideally you'd just discard them once they passed their best-before date.

Putting aside the time question (syncing of clocks) for one sec - if the caller passed in a time, then we could know how long it has been a) sitting around in network appliances, and b) how long it has been intra cougar. Stamping an inbound to cougar time would help with b, but not a.

Entire the client doing something for themselves. But whose time ..... you see how it unfolds from here.

On 27 November 2013 23:15, James D notifications@github.com wrote:

Why do you want clients to be specifying a timeout anyway? Why shouldn't the server just dump all requests that are older than so-many-millis or whatever? Perhaps discriminating by type of message. For example operation O1 has contract that says it will be dumped if over T1 millis at processing time, operation O2 will be dumped if over T2 millis... (could even say that O1 will be dumped after T1 if the request has such-and-such characteristics, and T2 if other characteristics).

I wonder whether you want a client telling you what to do. Might just be easier all around if the server tells everybody what it WILL do?

Bear in mind I don't know anything about who raised this requirement, why, or how much actual real life benefit it will bring to anybody. Is it just somebody's pet brainchild that sounds nice to have, or does it solve a very real problem?

— Reply to this email directly or view it on GitHubhttps://github.com/betfair/cougar/issues/55#issuecomment-29428196 .

eswdd commented 10 years ago

The client can already specify a timeout on the sync interface, and the server now has a timeout capability to protect itself within the EV, but we don't have anything to protect ingress to the EV queue. We do now have a hook to do this.

The time of dispatch piece is required to deal with the LB as @richardqd says above, it would be desirable if we also took into account how long the client is going to wait around as well.

eswdd commented 10 years ago

Ok, I think I can see a way forwards that leaves the flexibility for a better time source if one exists:

Thoughts?

atropineal commented 10 years ago

Yes, I understand. I was saying that if a network device can introduce a timestamp at the boundary of the system (e.g. public facing netscaler) like you previously suggested (covering point a), why bother with the complexity of having the client send something.

On Thu, Nov 28, 2013 at 1:28 AM, richardqd notifications@github.com wrote:

A very real problem we experienced during a recent bingle was that queued requests were held in a 50k+ item netscaler queue. This isn't good because consider any time sensitive requests in that queue, by the time the answer was calculated, it was irrelevant to the caller anyway. Worse, the service under the cosh had to go on and service those 50k+ requests anyway, even though the results will almost certainly have been discarded by the caller. Ideally you'd just discard them once they passed their best-before date.

Putting aside the time question (syncing of clocks) for one sec - if the caller passed in a time, then we could know how long it has been a) sitting around in network appliances, and b) how long it has been intra cougar. Stamping an inbound to cougar time would help with b, but not a.

Entire the client doing something for themselves. But whose time ..... you see how it unfolds from here.

On 27 November 2013 23:15, James D notifications@github.com wrote:

Why do you want clients to be specifying a timeout anyway? Why shouldn't the server just dump all requests that are older than so-many-millis or whatever? Perhaps discriminating by type of message. For example operation O1 has contract that says it will be dumped if over T1 millis at processing time, operation O2 will be dumped if over T2 millis... (could even say that O1 will be dumped after T1 if the request has such-and-such characteristics, and T2 if other characteristics).

I wonder whether you want a client telling you what to do. Might just be easier all around if the server tells everybody what it WILL do?

Bear in mind I don't know anything about who raised this requirement, why, or how much actual real life benefit it will bring to anybody. Is it just somebody's pet brainchild that sounds nice to have, or does it solve a very real problem?

— Reply to this email directly or view it on GitHub< https://github.com/betfair/cougar/issues/55#issuecomment-29428196> .

— Reply to this email directly or view it on GitHubhttps://github.com/betfair/cougar/issues/55#issuecomment-29428810 .

eswdd commented 10 years ago

In case you don't have a network device with that capability, or you know that all requests from a particular ip range are NTP'd (e.g. internal binary transport traffic) or some other criteria..

atropineal commented 10 years ago

If they're point to point binary requests then there's no network device surge queue though, no? And we DO have a network device with that capability. Why would we spend time and money developing features we don't need?

On Thu, Nov 28, 2013 at 11:09 AM, Simon Matic Langford < notifications@github.com> wrote:

In case you don't have a network device with that capability, or you know that all requests from a particular ip range are NTP'd (e.g. internal binary transport traffic) or some other criteria..

— Reply to this email directly or view it on GitHubhttps://github.com/betfair/cougar/issues/55#issuecomment-29448637 .

richardqd commented 10 years ago

Its not just about an external surge queue. If you have an extremely busy service, and your request is queued internally, the same rational for wanting to discard applies - a client is indicating that it is not interested in reply if it takes longer than xxx to produce. If our cougar service is that busy, then this helps protect both parties

On 28 November 2013 09:12, James D notifications@github.com wrote:

If they're point to point binary requests then there's no network device surge queue though, no? And we DO have a network device with that capability. Why would we spend time and money developing features we don't need?

On Thu, Nov 28, 2013 at 11:09 AM, Simon Matic Langford < notifications@github.com> wrote:

In case you don't have a network device with that capability, or you know that all requests from a particular ip range are NTP'd (e.g. internal binary transport traffic) or some other criteria..

— Reply to this email directly or view it on GitHub< https://github.com/betfair/cougar/issues/55#issuecomment-29448637> .

— Reply to this email directly or view it on GitHubhttps://github.com/betfair/cougar/issues/55#issuecomment-29448808 .

atropineal commented 10 years ago

I understand. Going back to the point I made earlier, is it not simpler to have the service explain its own general timeout policy, rather than having clients set pre-request policies.

On Thu, Nov 28, 2013 at 11:15 AM, richardqd notifications@github.comwrote:

Its not just about an external surge queue. If you have an extremely busy service, and your request is queued internally, the same rational for wanting to discard applies - a client is indicating that it is not interested in reply if it takes longer than xxx to produce. If our cougar service is that busy, then this helps protect both parties

On 28 November 2013 09:12, James D notifications@github.com wrote:

If they're point to point binary requests then there's no network device surge queue though, no? And we DO have a network device with that capability. Why would we spend time and money developing features we don't need?

On Thu, Nov 28, 2013 at 11:09 AM, Simon Matic Langford < notifications@github.com> wrote:

In case you don't have a network device with that capability, or you know that all requests from a particular ip range are NTP'd (e.g. internal binary transport traffic) or some other criteria..

— Reply to this email directly or view it on GitHub< https://github.com/betfair/cougar/issues/55#issuecomment-29448637> .

— Reply to this email directly or view it on GitHub< https://github.com/betfair/cougar/issues/55#issuecomment-29448808> .

— Reply to this email directly or view it on GitHubhttps://github.com/betfair/cougar/issues/55#issuecomment-29448971 .

atropineal commented 10 years ago

1) We (the company) have a network device that can stamp the request, for load-balanced transports 2) For point to point transports the time can be stamped on arrival 3) A server that expresses its own request timeout policy seems to me to be 'good enough' for load shedding purposes

OK, so who specifically is asking for per-request timeouts? What benefit will it bring to them? What benefit does it bring to us, the company, its shareholders? Is the cost to us worth the benefit to us? I'm interested.

On Thu, Nov 28, 2013 at 11:21 AM, James Dodd dodderish@gmail.com wrote:

I understand. Going back to the point I made earlier, is it not simpler to have the service explain its own general timeout policy, rather than having clients set pre-request policies.

On Thu, Nov 28, 2013 at 11:15 AM, richardqd notifications@github.comwrote:

Its not just about an external surge queue. If you have an extremely busy service, and your request is queued internally, the same rational for wanting to discard applies - a client is indicating that it is not interested in reply if it takes longer than xxx to produce. If our cougar service is that busy, then this helps protect both parties

On 28 November 2013 09:12, James D notifications@github.com wrote:

If they're point to point binary requests then there's no network device surge queue though, no? And we DO have a network device with that capability. Why would we spend time and money developing features we don't need?

On Thu, Nov 28, 2013 at 11:09 AM, Simon Matic Langford < notifications@github.com> wrote:

In case you don't have a network device with that capability, or you know that all requests from a particular ip range are NTP'd (e.g. internal binary transport traffic) or some other criteria..

— Reply to this email directly or view it on GitHub< https://github.com/betfair/cougar/issues/55#issuecomment-29448637> .

— Reply to this email directly or view it on GitHub< https://github.com/betfair/cougar/issues/55#issuecomment-29448808> .

— Reply to this email directly or view it on GitHubhttps://github.com/betfair/cougar/issues/55#issuecomment-29448971 .

bfmike commented 10 years ago

I see the server timeout as critical to protecting the service from stale requests. I can see some value in the client requesting a specific timeout (albeit less than the server's policy) especially if the timeout requirements cascade down through subsequent services.

If a client really needs an answer within 100ms then it should handle it's own timeout policy locally (because IP packets go missing and they will still experience situations where a socket hangs).

And seriously, don't ever think about relying on NTP - it was never designed to give perfect across all hosts (not even in the same DC). It is not worth the hassle and without really good monitoring you will hit situations where clock drift causes major problems within the application where there was nothing actually wrong within the infrastructure.

jamiei commented 10 years ago

I'm not sure it's necessarily as granular as per request, but I can see value in different time-outs for each client rather than per op.

For example, one could imagine a service with a broad range of clients with varying interests, some of whom request once a second or less frequently and some of whom request many times a second. You would want to offer a time-out that satisfies the requirements of the latter without cutting off the former inadvertently during times of high load. Having said that, I appreciate that if you're requesting infrequently, the occasional timed out request is probably not of significant consequence anyway.

atropineal commented 10 years ago

I'm not sure it's necessarily as granular as per request, but I can see value in different time-outs for each client rather than per op.

OK, well that can be a property of the application then (we already have the setup for that in the form of appkeys).

I'm still not seeing a good case for adding the complexity of client-specified timeouts, myself.

On Thu, Nov 28, 2013 at 12:14 PM, Jamie notifications@github.com wrote:

I'm not sure it's necessarily as granular as per request, but I can see value in different time-outs for each client rather than per op.

For example, one could imagine a service with a broad range of clients with varying interests, some of whom request once a second or less frequently and some of whom request many times a second. You would want to offer a time-out that satisfies the requirements of the latter without cutting off the former inadvertently during times of high load. Having said that, I appreciate that if you're requesting infrequently, the occasional timed out request is probably not of significant consequence anyway.

— Reply to this email directly or view it on GitHubhttps://github.com/betfair/cougar/issues/55#issuecomment-29453257 .

eswdd commented 10 years ago

The client already can specify and handles it's timeouts, it's part of the client interface, so there's no complexity being added, I'm just proposing sending it to the server so if the client is going to go away before us then we may as well stop earlier.

If we can't rely on NTP for situations where you care about the clock being in sync, then what can we rely on? The requirement for knowing (in the server's time) the time the client request was issued (or at least hit the server's network) is solid. To a certain extent, given the way I'm planning to implement, this doesn't really matter. I'll provide a hook and if someone finds a great solution then they can share it...

eswdd commented 10 years ago

Oh, and who is "we (the company)" in the context of an open source project? :P

Bear in mind the main person actively working on this feature is employed by a company that doesn't even use Java.

bfmike commented 10 years ago

"THE COMPANY" - the CIA dude!

bfmike commented 10 years ago

Oh, and on a more serious note: the load balancer is at fault here and shouldn't hold onto requests in a surge queue for longer than a configurable period of time.. naughty load balancer..

eswdd commented 10 years ago

You're rather assuming: a) It can be configured b) It has been configured

bfmike commented 10 years ago

More assuming that it CAN'T be configured or otherwise I would really hope that someone would have tried that before resorting to writing code.

Anyway, I can't spend all day trolling open source projects - I've got real work to do :-o

atropineal commented 10 years ago

Well, I'd say that the client sending a time to the server means extra complexity and opens up a can o worms, and for little benefit (from my own limited point of view). And with that, I'll butt out. :-)

I thought someone from the CIA, urr I mean BF had requested this feature.

On Thu, Nov 28, 2013 at 6:49 PM, bfmike notifications@github.com wrote:

More assuming that it CAN'T be configured or otherwise I would _really_hope that someone would have tried that before resorting to writing code.

Anyway, I can't spend all day trolling open source projects - I've got real work to do :-o

— Reply to this email directly or view it on GitHubhttps://github.com/betfair/cougar/issues/55#issuecomment-29475370 .

eswdd commented 10 years ago

Yeah, can o worms fair enough. I'm going to change the client to be have a hook to be able to send it across (reverse time resolver c.f. identity token resolution) and leave it at that..

I think Mr JS originally requested and I enhanced it on the way through.. :D

eswdd commented 10 years ago

Code done, will write a gist to cover what will be documented and then this will be done.