On 3 January 2014 14:45, Philippe Mouawad <philippe.moua...@gmail.com> wrote: > On Fri, Jan 3, 2014 at 3:33 PM, sebb <seb...@gmail.com> wrote: > >> On 3 January 2014 13:47, Philippe Mouawad <philippe.moua...@gmail.com> >> wrote: >> > Hello Sebb, >> > As requested although I don't see why it cannot be in bugzilla,if it were >> > the history of discussion could be found more easily. >> >> Because discussion may take a while and Bugzilla is not good for that. >> We can add links to the mail thread with any followup, once decisions are >> made. >> >> > Regards >> > Philippe >> > On Fri, Jan 3, 2014 at 1:57 PM, <bugzi...@apache.org> wrote: >> > >> >> https://issues.apache.org/bugzilla/show_bug.cgi?id=55932 >> >> >> >> --- Comment #6 from Sebb <s...@apache.org> --- >> >> I have been having a look at the implementation. >> >> >> >> I don't really see that it needs Commons Math; we aleady have >> >> StatCalculator >> >> which handles percentiles and more. >> >> >> > Ok I can change this. >> > >> >> >> >> Likewise, does it really need Commons Pool? >> >> It seems wrong to have to have 2 separate pools of SocketOutputStream >> >> instances. >> >> >> > Can you clarify ? >> > There is an executor pool (max size : 1 for now) and a >> > >> > socketOutputStreamPool in GraphiteMetricsManager. >> >> Sorry, that was wrong. >> >> There's a socketOutputStreamPool in PickleMetricsManager only >> I thought I had spotted another in SocketOutputStreamPoolFactory but >> that is just the implementation. >> >> > >> > How many of these would there be? >> >> >> > >> > Currently it is true we could use only one socket and keep it open. >> > >> > >> >> >> >> Also, DescriptiveStatistics is not thread-safe (nor is StatCalculator). >> >> >> > It is not a problem, as DescriptiveStatistics is accessed synchronously. >> >> Only write accesses are synchronised. >> But read needs to be synchronised as well to ensure safe publication. >> >> >> >> >> If we do implement something like this, I think the data processing >> needs >> >> either to be carefully synchronised, or the raw data should be sent to a >> >> separate singleton background thread. >> >> >> > >> > I think it is carefully synchronized in the patch. If not please point me >> > to where you see an issue. >> >> See above. >> >> If it used a background thread with a suitable queuing mechanism, >> there would be no need to synch the data collection, and the >> processing would not slow the main thread. >> > So you mean we publish the SampleResult in a queue and a thread handles > computation ?
Yes. We would need to define a suitable API (interface) here so that different implementations can be plugged in. > Do you intend to implement a kind of RingBuffer , something like this : > https://github.com/LMAX-Exchange/disruptor > or much simpler ? I was thinking of one of the standard Java queues like we already use for returning samples in client server mode. But it might be worth looking at disruptor. > And is there another thread to send ? Probably not necessary, and might be counter-productive as it would likely need additonal sync and another queue. > Could you propose a patch change ? > > > >> >> What happens currently if the backend socket stalls or runs slowly? >> > It will only affect the publication . > The run method will be executed while the other one is still running , so > we could send same statistics twice, good catch. > > >> >> Also, decoupling in this way would make it easier to provide >> implementations, as they would not need to be thread-safe. >> >> > >> >> >> >> Follow-ups to the dev list please. >> >> >> >> -- >> >> You are receiving this mail because: >> >> You are the assignee for the bug. >> >> >> > >> > >> > >> > -- >> > Cordialement. >> > Philippe Mouawad. >> > > > > -- > Cordialement. > Philippe Mouawad.
