Re: [Bug 55932] Create a Graphite Listener

[Philippe Mouawad <ph...@gmail.com>]

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.

Regards
Philippe
On Fri, Jan 3, 2014 at 1:57 PM, <bu...@apache.org> wrote:

> https://issues.apache.org/bugzilla/show_bug.cgi?id=55932
>
> --- Comment #6 from Sebb <se...@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.


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.

>
> 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.


>
> Follow-ups to the dev list please.
>
> --
> You are receiving this mail because:
> You are the assignee for the bug.
>



-- 
Cordialement.
Philippe Mouawad.

Re: [Bug 55932] Create a Graphite Listener

[sebb <se...@gmail.com>]

On 3 January 2014 14:45, Philippe Mouawad <ph...@gmail.com> wrote:
> On Fri, Jan 3, 2014 at 3:33 PM, sebb <se...@gmail.com> wrote:
>
>> On 3 January 2014 13:47, Philippe Mouawad <ph...@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, <bu...@apache.org> wrote:
>> >
>> >> https://issues.apache.org/bugzilla/show_bug.cgi?id=55932
>> >>
>> >> --- Comment #6 from Sebb <se...@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.

Re: [Bug 55932] Create a Graphite Listener

[Philippe Mouawad <ph...@gmail.com>]

On Fri, Jan 3, 2014 at 3:33 PM, sebb <se...@gmail.com> wrote:

> On 3 January 2014 13:47, Philippe Mouawad <ph...@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, <bu...@apache.org> wrote:
> >
> >> https://issues.apache.org/bugzilla/show_bug.cgi?id=55932
> >>
> >> --- Comment #6 from Sebb <se...@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 ?
Do you intend to implement a kind of RingBuffer , something like this :
https://github.com/LMAX-Exchange/disruptor
or much simpler ?
And is there another thread to send ?
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.

Re: [Bug 55932] Create a Graphite Listener

[sebb <se...@gmail.com>]

On 3 January 2014 13:47, Philippe Mouawad <ph...@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, <bu...@apache.org> wrote:
>
>> https://issues.apache.org/bugzilla/show_bug.cgi?id=55932
>>
>> --- Comment #6 from Sebb <se...@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.

What happens currently if the backend socket stalls or runs slowly?

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.