Commons Functor
  1. Commons Functor
  2. FUNCTOR-1

[functor] New components: summarize and aggregate

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.0
    • Labels:
    • Environment:

      JDK 1.6.0_25 but should work with any JDK 5+ (possibly 1.4 though I haven't tested).

      Description

      This is the next step from https://issues.apache.org/jira/browse/SANDBOX-340 – as instructed I'm finally hoping to get the code in the right place and hopefully this is something that the functor component could do with.
      Whereas initially I just started with the summarizer component, I have added now the second one, the "aggregator" as they are somehow related. If this code proves to be useful to functor in any way, it would actually be good to get some feedback on these 2 to see if the class hierarchy can in fact be changed to share some common functionality as I feel (probably due to the similar needs that lead to writing/using these components) that somehow they should share a common base.
      In brief, the 2 components:

      • aggregator: this just allows for data to be aggregated in a user defined way (e.g. stored in a list for the purpose of averaging, computing the arithmetic median etc). The classes provided actually offer the implementation for storing data in a list and computing the above-mentioned values or summing up everything.
      • timed summarizer: this is another variation of the aggreator, however, it adds the idea of regular "flushes", so based on a timer it will reset the value and start summing/aggregating the data again. Rather than using an aggregator which would store the whole data series (possibly for applying more complex formulas), this component just computes on the fly on each request the formula and stores the result of it. (Which does mean things like computing arithmetic mean, median etc would be difficult to compute without knowing upfront how many calls will be received – i.e. how many elements we will be required to summarize/aggregate.) So the memory footprint of running this is much smaller – even though, as I said, it achieves similar results. I have only provided a summarizer which operates on integers, but obviously others for float, double etc can be created if we go ahead with this design.
        Hopefully the above make sense; this code has resulted from finding myself writing similar components to these a few times and because it's always been either one type (e.g. aggregator) or another (summarizer) I haven't given quite possibly enough thought to the class design to join these 2. Also, unfortunately, the time I sat down to make these components a bit more general and submitted issue 340 was nearly 3 months ago so I'm trying to remember myself all the ideas I had at a time so bear with me please if these are still a bit fuzzy However, if you can make use of these I'm quite happy to elaborate on areas that are unclear and obviously put some effort into getting these components to the standards required to put these into a release.
      1. commons-functor-aggregate+summarizer.zip
        5 kB
        Liviu Tudor
      2. commons-functor.patch.bz2
        14 kB
        Liviu Tudor
      3. functor.patch.bz2
        18 kB
        Liviu Tudor
      4. functor.patch.bz2
        22 kB
        Liviu Tudor
      5. functor.22-may-2012.patch.bz2
        23 kB
        Liviu Tudor

        Activity

        Hide
        Simone Tripodi added a comment -

        Patch (finally) applied, see r1344757

        Thanks a lot Liviu for the large codebase, and thanks also Bruno for reviewing and providing help!!!

        Show
        Simone Tripodi added a comment - Patch (finally) applied, see r1344757 Thanks a lot Liviu for the large codebase, and thanks also Bruno for reviewing and providing help!!!
        Hide
        Liviu Tudor added a comment -

        Patch for my reply to Bruno

        Show
        Liviu Tudor added a comment - Patch for my reply to Bruno
        Hide
        Liviu Tudor added a comment -

        Hi Liviu!

        First of all, it's very clear you have put a lot of effort on this patch. I've applied at a local working copy of functor trunk, and it is very neat indeed. I wish my patches were as good as yours

        Thank you! To be honest I think the quality of my previous patch was well under average on that – I am still wondering how Simone had so much patience to actually go through it – so I thought I'd put some serious effort in making it at least readable, so at least if this patch turns out to be a dud people don't waste too much time on trying to figure it out

        I'm starting to learn about aggregators now, while reading the code and documents that you wrote. So feel free to correct or comment any of my points below.

        • The documentation and examples provided are very good However, I think you used <br/> to break line between paragraph, while the other examples use <p>. The visual is a bit different.

        Very good point! I never picked on that myself – I'm a sucker for closing and opening tags (e.g. <p> ..... </p>) so out of sheer laziness I guess I find myself using the <br/> a lot I went through the code and replaced all the <br/> with <p> so it should be all uniform now. (Wouldn't mind a second pair of eyes to verify that I haven't missed a <br/> though, if you have the time!)

        • After I read the issue and applied the patch (I had a quick view on it before, wow, so many tests I opened Eclipse and looked for the aggregator package, but it's aggregate. I think functor, generator and adapter package names follow the name of the pattern they implement. And I have seen aggregate and aggregator as the name of the pattern. What do you think?

        You have a fair point, again – the "aggregate" name stems really from the first patch sent which was quite different to what "aggregator" has become in the last patch I've submitted. It somehow never crossed my mind after the refactoring(s) to change the package to "aggregator" – though now that you mentioned it, it makes perfect sense! So I went ahead and moved packages under "aggregator".

        • How does an aggregator differs from the reduce function (like in map reduce)? IIUC, aggregators are like the fold function. A "nostore aggregator" is more like a fold with recursion, while the list aggregator is a fold function which stores its partial value somewhere. Maybe we could use FoldLeft with aggregators?

        I think you might be right – gonna go and have a look at FoldLeft – haven't considered that approach to be honest (nor am I that familiar with the class itself – though I am aware of the whole map/reduce paradigm).

        • In the o.a.c.functor.aggregate.functions package, there are several aggregator functors for Double and Integer. The name of aggregators for Double start with Double. But the name of aggregators for Integer start with Int. In o.a.c.functor.generator.util, there are LongRange and IntegerRange. So I think in this case we could put both as Int, or both as Integer. What do you think?

        My laziness strikes again I don't like typing long class names – even though I'm an Eclipse user and make full usage of all the wonders of autocomplete etc, long class names don't sit well with me (showing my old age here, see, I used a lot of "dumb" editors in my time when terminals were green and black – hence my choosing Int versus Integer. However, that could be misleading : Integer suggest java.lang.Integer wheres Int in the name could suggest the primitive int.
        Since we already used the convention of Integer for when java.lang.Integer is used in functor, I went and changed all the names to include Integer rather than just Int. Thanks for the heads-up on this!

        • IIUC, in o.a.c.functor.aggregate there are classes for creating nostore and list aggregators. However, if I create a ArrayListBackedAggregator<T>, this aggregator extends AbstractTimedAggregator<T>. What means AbsractTimedAggregator<Integer> aggr = new ArrayListBackedAggregator<Integer>(); would be valid (not tested, but I think it works). I think these classes could be designed in some other way to avoid this scenario... just food for thought

        Yes, that is the intended behaviour: in other words the implementations provided all are meant to have built-in timer support. I have defined the top-level interface Aggregator with no timer support as there might be other scenarios the Aggregator can be used in, which don't require timer support (even though that can be achieved with a null timer in AbstractTimedAggregator).
        Basically this package appeared from finding myself quite often writing similar aggregators for various apps or components I was working on – and every time I needed this data to be flushed into a sink (typically logged in a log file or over a socket) regularly. As I said, this isn't to say Aggregator must be used in a timer context – hence, as I said, the Aggregator interface doesn't make any reference to that.
        Do you think these interfaces / classes should be structured some other way then? (Or is it perhaps the fact the naming of classes and interfaces is it not the best?)

        • AbstractTimedAggregator<T> implements TimedAggregator<T>. But in TimedAggregator#onTimer, the first parameter is an AbstractTimedAggregator. I think this could be replaced by a TimedAggregator. What do you think? Otherwise, it would be like the interface had a dependency to a class that implements it. And if another class implemented TimedAggregator, it would have a dependency to AbstractTimedAggregator too. Or if AbstractTimedAggregator was deprecated/removed, it would cause problems in TimedAggregator interface.

        There is no interface TimedAggregator – it's just Aggregator and AbstractTimedAggregator implements it. The TimedAggregatorListener indeed has a dependency on the AbstractTimedAggregator class which I agree is not the best – perhaps, a reason for introducing a TimedAggregator interface and work at interface level in TimedAggregatorListener?
        Though at the same time, the only reason why AbstractTimedAggregator is mentioned in TimedAggregatorListener is because the base interface Aggregator has no concept or mention of timer, so while it is possible to have TimedAggregatorListener declare a method like this:

        void onTimer(Aggregator<T> aggregator, T evaluation);
        

        It just didn't make sense to me, since like I said, Aggregator doesn't have a concept of timer.
        So the more I think about it, I could introduce a TimedAggregator interface which would extend Aggregator and offer 3 other methods:

        • addListener
        • removeListener
        • timer
          What do you think? Or should we use Aggregator in TimedAggregatorListener?

        The docs, examples and coverage are great. No issues in PMD, FindBugs or Checkstyle found so far. All licenses in place, as well as package javadoc. Next time I submit a patch I hope it can as neat as yours

        Thank you!

        There is an issue in Google Guava for similar feature, have you seen it (http://code.google.com/p/guava-libraries/issues/detail?id=546)?

        I wasn't aware of it to be honest. I'll try to have a look at it but to be honest, I fear timing is not my most abundant resource and I'd rather spend most of the time coding functor I'll have a look though, just to see what they came up with.

        I'm attaching an update to the patch (labelled functor.22-may-2012.patch.bz2) with the above changes – would appreciate your comments on this!

        Show
        Liviu Tudor added a comment - Hi Liviu! First of all, it's very clear you have put a lot of effort on this patch. I've applied at a local working copy of functor trunk, and it is very neat indeed. I wish my patches were as good as yours Thank you! To be honest I think the quality of my previous patch was well under average on that – I am still wondering how Simone had so much patience to actually go through it – so I thought I'd put some serious effort in making it at least readable, so at least if this patch turns out to be a dud people don't waste too much time on trying to figure it out I'm starting to learn about aggregators now, while reading the code and documents that you wrote. So feel free to correct or comment any of my points below. The documentation and examples provided are very good However, I think you used <br/> to break line between paragraph, while the other examples use <p>. The visual is a bit different. Very good point! I never picked on that myself – I'm a sucker for closing and opening tags (e.g. <p> ..... </p>) so out of sheer laziness I guess I find myself using the <br/> a lot I went through the code and replaced all the <br/> with <p> so it should be all uniform now. (Wouldn't mind a second pair of eyes to verify that I haven't missed a <br/> though, if you have the time!) After I read the issue and applied the patch (I had a quick view on it before, wow, so many tests I opened Eclipse and looked for the aggregator package, but it's aggregate. I think functor, generator and adapter package names follow the name of the pattern they implement. And I have seen aggregate and aggregator as the name of the pattern. What do you think? You have a fair point, again – the "aggregate" name stems really from the first patch sent which was quite different to what "aggregator" has become in the last patch I've submitted. It somehow never crossed my mind after the refactoring(s) to change the package to "aggregator" – though now that you mentioned it, it makes perfect sense! So I went ahead and moved packages under "aggregator". How does an aggregator differs from the reduce function (like in map reduce)? IIUC, aggregators are like the fold function. A "nostore aggregator" is more like a fold with recursion, while the list aggregator is a fold function which stores its partial value somewhere. Maybe we could use FoldLeft with aggregators? I think you might be right – gonna go and have a look at FoldLeft – haven't considered that approach to be honest (nor am I that familiar with the class itself – though I am aware of the whole map/reduce paradigm). In the o.a.c.functor.aggregate.functions package, there are several aggregator functors for Double and Integer. The name of aggregators for Double start with Double. But the name of aggregators for Integer start with Int. In o.a.c.functor.generator.util, there are LongRange and IntegerRange. So I think in this case we could put both as Int, or both as Integer. What do you think? My laziness strikes again I don't like typing long class names – even though I'm an Eclipse user and make full usage of all the wonders of autocomplete etc, long class names don't sit well with me (showing my old age here, see, I used a lot of "dumb" editors in my time when terminals were green and black – hence my choosing Int versus Integer. However, that could be misleading : Integer suggest java.lang.Integer wheres Int in the name could suggest the primitive int. Since we already used the convention of Integer for when java.lang.Integer is used in functor, I went and changed all the names to include Integer rather than just Int. Thanks for the heads-up on this! IIUC, in o.a.c.functor.aggregate there are classes for creating nostore and list aggregators. However, if I create a ArrayListBackedAggregator<T>, this aggregator extends AbstractTimedAggregator<T>. What means AbsractTimedAggregator<Integer> aggr = new ArrayListBackedAggregator<Integer>(); would be valid (not tested, but I think it works). I think these classes could be designed in some other way to avoid this scenario... just food for thought Yes, that is the intended behaviour: in other words the implementations provided all are meant to have built-in timer support. I have defined the top-level interface Aggregator with no timer support as there might be other scenarios the Aggregator can be used in, which don't require timer support (even though that can be achieved with a null timer in AbstractTimedAggregator). Basically this package appeared from finding myself quite often writing similar aggregators for various apps or components I was working on – and every time I needed this data to be flushed into a sink (typically logged in a log file or over a socket) regularly. As I said, this isn't to say Aggregator must be used in a timer context – hence, as I said, the Aggregator interface doesn't make any reference to that. Do you think these interfaces / classes should be structured some other way then? (Or is it perhaps the fact the naming of classes and interfaces is it not the best?) AbstractTimedAggregator<T> implements TimedAggregator<T>. But in TimedAggregator#onTimer, the first parameter is an AbstractTimedAggregator. I think this could be replaced by a TimedAggregator. What do you think? Otherwise, it would be like the interface had a dependency to a class that implements it. And if another class implemented TimedAggregator, it would have a dependency to AbstractTimedAggregator too. Or if AbstractTimedAggregator was deprecated/removed, it would cause problems in TimedAggregator interface. There is no interface TimedAggregator – it's just Aggregator and AbstractTimedAggregator implements it. The TimedAggregatorListener indeed has a dependency on the AbstractTimedAggregator class which I agree is not the best – perhaps, a reason for introducing a TimedAggregator interface and work at interface level in TimedAggregatorListener ? Though at the same time, the only reason why AbstractTimedAggregator is mentioned in TimedAggregatorListener is because the base interface Aggregator has no concept or mention of timer, so while it is possible to have TimedAggregatorListener declare a method like this: void onTimer(Aggregator<T> aggregator, T evaluation); It just didn't make sense to me, since like I said, Aggregator doesn't have a concept of timer. So the more I think about it, I could introduce a TimedAggregator interface which would extend Aggregator and offer 3 other methods: addListener removeListener timer What do you think? Or should we use Aggregator in TimedAggregatorListener ? The docs, examples and coverage are great. No issues in PMD, FindBugs or Checkstyle found so far. All licenses in place, as well as package javadoc. Next time I submit a patch I hope it can as neat as yours Thank you! There is an issue in Google Guava for similar feature, have you seen it ( http://code.google.com/p/guava-libraries/issues/detail?id=546)? I wasn't aware of it to be honest. I'll try to have a look at it but to be honest, I fear timing is not my most abundant resource and I'd rather spend most of the time coding functor I'll have a look though, just to see what they came up with. I'm attaching an update to the patch (labelled functor.22-may-2012.patch.bz2 ) with the above changes – would appreciate your comments on this!
        Hide
        Bruno P. Kinoshita added a comment -

        Hi Liviu!

        First of all, it's very clear you have put a lot of effort on this patch. I've applied at a local working copy of functor trunk, and it is very neat indeed. I wish my patches were as good as yours

        I'm starting to learn about aggregators now, while reading the code and documents that you wrote. So feel free to correct or comment any of my points below.

        • The documentation and examples provided are very good However, I think you used <br/> to break line between paragraph, while the other examples use <p>. The visual is a bit different.
        • After I read the issue and applied the patch (I had a quick view on it before, wow, so many tests I opened Eclipse and looked for the aggregator package, but it's aggregate. I think functor, generator and adapter package names follow the name of the pattern they implement. And I have seen aggregate and aggregator as the name of the pattern. What do you think?
        • How does an aggregator differs from the reduce function (like in map reduce)? IIUC, aggregators are like the fold function. A "nostore aggregator" is more like a fold with recursion, while the list aggregator is a fold function which stores its partial value somewhere. Maybe we could use FoldLeft with aggregators?
        • In the o.a.c.functor.aggregate.functions package, there are several aggregator functors for Double and Integer. The name of aggregators for Double start with Double. But the name of aggregators for Integer start with Int. In o.a.c.functor.generator.util, there are LongRange and IntegerRange. So I think in this case we could put both as Int, or both as Integer. What do you think?
        • IIUC, in o.a.c.functor.aggregate there are classes for creating nostore and list aggregators. However, if I create a ArrayListBackedAggregator<T>, this aggregator extends AbstractTimedAggregator<T>. What means AbsractTimedAggregator<Integer> aggr = new ArrayListBackedAggregator<Integer>(); would be valid (not tested, but I think it works). I think these classes could be designed in some other way to avoid this scenario... just food for thought
        • AbstractTimedAggregator<T> implements TimedAggregator<T>. But in TimedAggregator#onTimer, the first parameter is an AbstractTimedAggregator. I think this could be replaced by a TimedAggregator. What do you think? Otherwise, it would be like the interface had a dependency to a class that implements it. And if another class implemented TimedAggregator, it would have a dependency to AbstractTimedAggregator too. Or if AbstractTimedAggregator was deprecated/removed, it would cause problems in TimedAggregator interface.

        The docs, examples and coverage are great. No issues in PMD, FindBugs or Checkstyle found so far. All licenses in place, as well as package javadoc. Next time I submit a patch I hope it can as neat as yours

        There is an issue in Google Guava for similar feature, have you seen it (http://code.google.com/p/guava-libraries/issues/detail?id=546)?

        Cheers
        Bruno

        Show
        Bruno P. Kinoshita added a comment - Hi Liviu! First of all, it's very clear you have put a lot of effort on this patch. I've applied at a local working copy of functor trunk, and it is very neat indeed. I wish my patches were as good as yours I'm starting to learn about aggregators now, while reading the code and documents that you wrote. So feel free to correct or comment any of my points below. The documentation and examples provided are very good However, I think you used <br/> to break line between paragraph, while the other examples use <p>. The visual is a bit different. After I read the issue and applied the patch (I had a quick view on it before, wow, so many tests I opened Eclipse and looked for the aggregator package, but it's aggregate. I think functor, generator and adapter package names follow the name of the pattern they implement. And I have seen aggregate and aggregator as the name of the pattern. What do you think? How does an aggregator differs from the reduce function (like in map reduce)? IIUC, aggregators are like the fold function. A "nostore aggregator" is more like a fold with recursion, while the list aggregator is a fold function which stores its partial value somewhere. Maybe we could use FoldLeft with aggregators? In the o.a.c.functor.aggregate.functions package, there are several aggregator functors for Double and Integer. The name of aggregators for Double start with Double. But the name of aggregators for Integer start with Int. In o.a.c.functor.generator.util, there are LongRange and IntegerRange. So I think in this case we could put both as Int, or both as Integer. What do you think? IIUC, in o.a.c.functor.aggregate there are classes for creating nostore and list aggregators. However, if I create a ArrayListBackedAggregator<T>, this aggregator extends AbstractTimedAggregator<T>. What means AbsractTimedAggregator<Integer> aggr = new ArrayListBackedAggregator<Integer>(); would be valid (not tested, but I think it works). I think these classes could be designed in some other way to avoid this scenario... just food for thought AbstractTimedAggregator<T> implements TimedAggregator<T>. But in TimedAggregator#onTimer, the first parameter is an AbstractTimedAggregator. I think this could be replaced by a TimedAggregator. What do you think? Otherwise, it would be like the interface had a dependency to a class that implements it. And if another class implemented TimedAggregator, it would have a dependency to AbstractTimedAggregator too. Or if AbstractTimedAggregator was deprecated/removed, it would cause problems in TimedAggregator interface. The docs, examples and coverage are great. No issues in PMD, FindBugs or Checkstyle found so far. All licenses in place, as well as package javadoc. Next time I submit a patch I hope it can as neat as yours There is an issue in Google Guava for similar feature, have you seen it ( http://code.google.com/p/guava-libraries/issues/detail?id=546)? Cheers Bruno
        Hide
        Liviu Tudor added a comment -

        Submitted new patch (had some time on my hands this Saturday which hopefully addresses the issues signalled by Simo.
        I have only provided implementations for Integer and Double – I could provide implementations for Float, Long, Byte as well though not sure if those are needed as one can easily revert (in most cases) to Integer/Double? Let me know how you feel about that.
        I've updated the documentation (aggregate.xml) and changed examples.xml to include a reference to this and also point to the 2 new packages I've included under test which contain a few Java sources showing various usages for this component.
        Looking forward to your feedback on this!

        Liv

        Show
        Liviu Tudor added a comment - Submitted new patch (had some time on my hands this Saturday which hopefully addresses the issues signalled by Simo. I have only provided implementations for Integer and Double – I could provide implementations for Float, Long, Byte as well though not sure if those are needed as one can easily revert (in most cases) to Integer/Double? Let me know how you feel about that. I've updated the documentation (aggregate.xml) and changed examples.xml to include a reference to this and also point to the 2 new packages I've included under test which contain a few Java sources showing various usages for this component. Looking forward to your feedback on this! Liv
        Hide
        Liviu Tudor added a comment -

        Updated patch as of 25/Feb/2012

        Show
        Liviu Tudor added a comment - Updated patch as of 25/Feb/2012
        Hide
        Liviu Tudor added a comment -

        Hi Simo,

        Another question: I have packaged the examples/docs for this under src/site/aggregate.xml – which ends up as site/aggregate.html, however this doesn't get linked from anywhere in the main "menus" – is there a "standard" way of adding this or just add a hardcoded link to it say under <menu name="Commons Functor">?

        Show
        Liviu Tudor added a comment - Hi Simo, Another question: I have packaged the examples/docs for this under src/site/aggregate.xml – which ends up as site/aggregate.html, however this doesn't get linked from anywhere in the main "menus" – is there a "standard" way of adding this or just add a hardcoded link to it say under <menu name="Commons Functor">?
        Hide
        Liviu Tudor added a comment -

        Hi Simo,

        Thanks for your feedback – much appreciated!
        Indeed, I didn't supply the implementations for other primitives, as I was just checking to see if I'm on the right track, will add those in the next patch.
        I'll have a think about the naming – it made (some) sense to me, but you're right, it's probably not too descriptive
        I'll come back with another patch.
        thanks again,

        Liv

        Show
        Liviu Tudor added a comment - Hi Simo, Thanks for your feedback – much appreciated! Indeed, I didn't supply the implementations for other primitives, as I was just checking to see if I'm on the right track, will add those in the next patch. I'll have a think about the naming – it made (some) sense to me, but you're right, it's probably not too descriptive I'll come back with another patch. thanks again, Liv
        Hide
        Simone Tripodi added a comment -

        Hi Liviu,
        thanks for the contribution, patch looks good. I have anyway few observations before applying it:

        • the name *AggregatorFunction_2_ doesn't help since doesn't express any semantic. Can you change them in a more self descriptive names?
        • In the org.apache.commons.functor.aggregate.functions package I just see the implementation for Double and one Integer related class - maybe you forgot to svn add the other implementations?
        • Could you provide implementations also for other primitive wrappers?

        Documentaion under src/site/xdoc is more than fine and satisfies FUNCTOR-4 - feel free to enhance the doc adding more samples!

        Thanks for the hard work, looking forward the next patch version!
        -Simo

        Show
        Simone Tripodi added a comment - Hi Liviu, thanks for the contribution, patch looks good. I have anyway few observations before applying it: the name *AggregatorFunction_2_ doesn't help since doesn't express any semantic. Can you change them in a more self descriptive names? In the org.apache.commons.functor.aggregate.functions package I just see the implementation for Double and one Integer related class - maybe you forgot to svn add the other implementations? Could you provide implementations also for other primitive wrappers? Documentaion under src/site/xdoc is more than fine and satisfies FUNCTOR-4 - feel free to enhance the doc adding more samples! Thanks for the hard work, looking forward the next patch version! -Simo
        Hide
        Liviu Tudor added a comment -

        Question – looking at FUNCTOR-4 , I need to add some examples etc for these components to make it easier for users. Where would these examples go?

        Show
        Liviu Tudor added a comment - Question – looking at FUNCTOR-4 , I need to add some examples etc for these components to make it easier for users. Where would these examples go?
        Hide
        Liviu Tudor added a comment -

        Patch as per last comment.

        Show
        Liviu Tudor added a comment - Patch as per last comment.
        Hide
        Liviu Tudor added a comment -

        Hi Simone,

        I finally got around to doing the coding for this. Please have a look at the attached patch – I checked the findbugs/checkstyle as instructed and as far as I can tell it looks ok.
        Looking forward to your comments on it!

        Show
        Liviu Tudor added a comment - Hi Simone, I finally got around to doing the coding for this. Please have a look at the attached patch – I checked the findbugs/checkstyle as instructed and as far as I can tell it looks ok. Looking forward to your comments on it!
        Hide
        Liviu Tudor added a comment -

        Hi Simone,

        I've already replied to you in private, this is just for other maintainers to be aware of this: I'm currently in the process of relocating to Palo Alto, CA (anyone living in the Bay area up for an informal meet at some point? so things a bit hectic but still planning to finish this. ETA is about 2 weeks – so end of October.
        Hope this is ok,

        Liv

        Show
        Liviu Tudor added a comment - Hi Simone, I've already replied to you in private, this is just for other maintainers to be aware of this: I'm currently in the process of relocating to Palo Alto, CA (anyone living in the Bay area up for an informal meet at some point? so things a bit hectic but still planning to finish this. ETA is about 2 weeks – so end of October. Hope this is ok, Liv
        Hide
        Simone Tripodi added a comment -

        Hi Liviu,
        how things are going? Do you need any help from my side to finalize it?
        TIA,
        Simo

        Show
        Simone Tripodi added a comment - Hi Liviu, how things are going? Do you need any help from my side to finalize it? TIA, Simo
        Hide
        Liviu Tudor added a comment -

        Hi Simone, thanks for the heads-up on this, sorry, been away last week only just got back. Will go through the code and your comments and update the code.

        Show
        Liviu Tudor added a comment - Hi Simone, thanks for the heads-up on this, sorry, been away last week only just got back. Will go through the code and your comments and update the code.
        Hide
        Simone Tripodi added a comment -

        Hi Liviu,
        please update your local copy of the code, I recently ported the tests to JUnit4 style.

        Show
        Simone Tripodi added a comment - Hi Liviu, please update your local copy of the code, I recently ported the tests to JUnit4 style.
        Hide
        Simone Tripodi added a comment -

        Hi Liviu!
        no pain at all - smart and volunteering people like you are lifeblood for community's health, so I am more than happy to put contributors in the position of working in the better way

        So, to produce those reports, it is enough you run mvn clean site && open target/site/checkstyle.html, it will produce a page like the one actually online[1].
        Then, if you open target/site/cobertura/index.html you can have a look at the generated cobertura report.
        Just take care of classes you put in the summarize and aggregate package, no need to take care of the rest - I'm doing it during the spare time, feel free to fill a new issue with a new patch anyway

        Documentation: take a look at existing pages, the xdoc format[2] is ~html, so not hard to write. And yes, just an overview of added classes, samples, patterns... under both PoVs of users (how to write client code) and developers (extending APIs, if allowed, etc)

        Pom: you did write, sorry for the mistake, actually {<contributors> is the right place where adding your name - hopefully you will become a committer

        Last: the right place where sharing thoughts and discussing is the dev@ ML, in Apache there's the mantra "if it didn't happen in the list, it didn't happen", JIRA is more like a "wall stories" reminder

        Looking forward for the next patch, have a nice day!

        [1] http://commons.apache.org/sandbox/functor/checkstyle.html
        [2] http://commons.apache.org/sandbox/functor/cobertura/index.html
        [3] http://maven.apache.org/doxia/references/xdoc-format.html

        Show
        Simone Tripodi added a comment - Hi Liviu! no pain at all - smart and volunteering people like you are lifeblood for community's health, so I am more than happy to put contributors in the position of working in the better way So, to produce those reports, it is enough you run mvn clean site && open target/site/checkstyle.html , it will produce a page like the one actually online [1] . Then, if you open target/site/cobertura/index.html you can have a look at the generated cobertura report. Just take care of classes you put in the summarize and aggregate package, no need to take care of the rest - I'm doing it during the spare time, feel free to fill a new issue with a new patch anyway Documentation: take a look at existing pages, the xdoc format [2] is ~html, so not hard to write. And yes, just an overview of added classes, samples, patterns... under both PoVs of users (how to write client code) and developers (extending APIs, if allowed, etc) Pom: you did write, sorry for the mistake, actually { <contributors> is the right place where adding your name - hopefully you will become a committer Last: the right place where sharing thoughts and discussing is the dev@ ML, in Apache there's the mantra "if it didn't happen in the list, it didn't happen", JIRA is more like a "wall stories" reminder Looking forward for the next patch, have a nice day! [1] http://commons.apache.org/sandbox/functor/checkstyle.html [2] http://commons.apache.org/sandbox/functor/cobertura/index.html [3] http://maven.apache.org/doxia/references/xdoc-format.html
        Hide
        Liviu Tudor added a comment -

        Hi Simone,

        Agreed on not needing the evaluate() function – really it was included there so I can have the JavaDoc section for it – but I've moved that in the class header.
        Will make the changes to use getters/setters for abstract classes; meanwhile though, to help me address all the checkstyle/findbugs issues, can you point me in the right direction as to how to run the findbugs/checkstyle against the code and where would I find the results? (sorry a bit new to the whole maven setup – and luckily in most environments i've used it so far, I relied on the automated hudson/integration to produce these nightly reports
        With regards to the document under src/site/xdocs – what should this include? A high level overview of the components and possibly including sample code maybe with patterns of usage? Or what things should I touch on?
        One last thing, going back through your comments, I've only just noticed a small discrepancy – you have asked me to add myself to the list of developers in the pom.xml – I opted to put myself on the contributors list as I'm guessing developers would be the Apache peeps actually maintaing/managing this project, right? Not a big thing really, just trying to iron out all the small details at the moment, so thought I'd ask about this as well.
        Sorry to be a pain!

        Show
        Liviu Tudor added a comment - Hi Simone, Agreed on not needing the evaluate() function – really it was included there so I can have the JavaDoc section for it – but I've moved that in the class header. Will make the changes to use getters/setters for abstract classes; meanwhile though, to help me address all the checkstyle/findbugs issues, can you point me in the right direction as to how to run the findbugs/checkstyle against the code and where would I find the results? (sorry a bit new to the whole maven setup – and luckily in most environments i've used it so far, I relied on the automated hudson/integration to produce these nightly reports With regards to the document under src/site/xdocs – what should this include? A high level overview of the components and possibly including sample code maybe with patterns of usage? Or what things should I touch on? One last thing, going back through your comments, I've only just noticed a small discrepancy – you have asked me to add myself to the list of developers in the pom.xml – I opted to put myself on the contributors list as I'm guessing developers would be the Apache peeps actually maintaing/managing this project, right? Not a big thing really, just trying to iron out all the small details at the moment, so thought I'd ask about this as well. Sorry to be a pain!
        Hide
        Simone Tripodi added a comment -

        Forgot to ask the favor of having a look also at generated findbugs/checkstyle reports and fix potential errors/violations in classes in the package you added.
        TIA!

        Show
        Simone Tripodi added a comment - Forgot to ask the favor of having a look also at generated findbugs/checkstyle reports and fix potential errors/violations in classes in the package you added. TIA!
        Hide
        Simone Tripodi added a comment -

        I had a look at the new patch, few more minor comments:

        • since Aggregator<T> now extends Function<T>, there's no need to redeclare the T evaluate() method;
        • in abstract implementations, Class members access has to be done via getters/setters instead of making fields protected (checkstyle would complain about it)

        Moreover, a favor: can you please create a documentation page under src/site/xdoc/? That would be useful to put users aware of such feature (and yes, the rest of documentation is still a TODO )

        Many thanks in advance for your hard work!

        Show
        Simone Tripodi added a comment - I had a look at the new patch, few more minor comments: since Aggregator<T> now extends Function<T> , there's no need to redeclare the T evaluate() method; in abstract implementations, Class members access has to be done via getters/setters instead of making fields protected (checkstyle would complain about it) Moreover, a favor: can you please create a documentation page under src/site/xdoc/ ? That would be useful to put users aware of such feature (and yes, the rest of documentation is still a TODO ) Many thanks in advance for your hard work!
        Hide
        Liviu Tudor added a comment -

        update code – 90% done (hopefully!)

        Show
        Liviu Tudor added a comment - update code – 90% done (hopefully!)
        Hide
        Liviu Tudor added a comment -

        OK so I reckon I'm 90% there so I'm just submitting this patch to see if I'm doing something right or what am I doing wrong.
        I've reworked the whole package structure to use UnaryFunction and BinaryFunction and as such I ended up with just one package (aggregator) and the summarizer becomes just an implementation of that (AbstractTimedAggregator). I'm still not 100% sure with having the AbstractListBackedAggregator and AbstractNoStoreAggregator extending AbstractTimedAggregator – and I wanted to run this past you guys to see what would be a more elegant solution?
        I need to provide unit tests for AbstractTimedAggregator and add a couple of more tests to the exiting ones to test for timer support as well but otherwise I think it's done.
        Would love some feedback and suggestions.

        Show
        Liviu Tudor added a comment - OK so I reckon I'm 90% there so I'm just submitting this patch to see if I'm doing something right or what am I doing wrong. I've reworked the whole package structure to use UnaryFunction and BinaryFunction and as such I ended up with just one package (aggregator) and the summarizer becomes just an implementation of that ( AbstractTimedAggregator ). I'm still not 100% sure with having the AbstractListBackedAggregator and AbstractNoStoreAggregator extending AbstractTimedAggregator – and I wanted to run this past you guys to see what would be a more elegant solution? I need to provide unit tests for AbstractTimedAggregator and add a couple of more tests to the exiting ones to test for timer support as well but otherwise I think it's done. Would love some feedback and suggestions.
        Hide
        Simone Tripodi added a comment -

        Hi Liviu!
        about the Apache header, just open existing functor classes and copy from them, it is more than enough

        It would be really appreciated if you could improve unit testing (take a look also to the generated code coverage report) otherwise, you can understand, people would be a little reluctant on adding non tested code...

        I'd tend to agree that passing an existing Timer would allow smarter users better managing their resources, go for it and see how things go

        Looking forward for the next patch!

        Show
        Simone Tripodi added a comment - Hi Liviu! about the Apache header, just open existing functor classes and copy from them, it is more than enough It would be really appreciated if you could improve unit testing (take a look also to the generated code coverage report) otherwise, you can understand, people would be a little reluctant on adding non tested code... I'd tend to agree that passing an existing Timer would allow smarter users better managing their resources, go for it and see how things go Looking forward for the next patch!
        Hide
        Liviu Tudor added a comment -

        Hi Simone,

        Thanks for the feedback – my first commit, so I expected a lot of things to go wrong.
        Since my code was initially committed to commons.lang, I didn't have access to the classes you mentioned, so I will have a look and use them.
        Secondly, regarding the Apache header, am I correct in assuming that simply copying and pasting it from an existing class in the repository should be enough? Point taken about the @author tag, that was generated automatically by my Eclipse (note to self: adjust my Eclipse templates to remove that and a few other things!).
        I didn't spend too much time on unit testing purely because I wasn't sure still if this was the right place to commit this component, now that I am on the right track I will spend some time to implement those properly.
        Last but not least, org.apache.commons.functor.summarizer.TimedSummarizer#MAIN_TIMER is static in order to avoid creating a new one for each instance – but you are right, it can create a memory leak. I will actually spend some time on it and probably provide a factory for creating instances of the TimedSummarizer class which will allow for the instance to use it's own Timer or a shared one; this is so "power users" can use the shared one and minimize the memory and threading footprint, while still allowing for Joe Average to avoid the memory leakage by using a per-instance Timer. Do you think that would be a good idea?

        Show
        Liviu Tudor added a comment - Hi Simone, Thanks for the feedback – my first commit, so I expected a lot of things to go wrong. Since my code was initially committed to commons.lang , I didn't have access to the classes you mentioned, so I will have a look and use them. Secondly, regarding the Apache header, am I correct in assuming that simply copying and pasting it from an existing class in the repository should be enough? Point taken about the @author tag, that was generated automatically by my Eclipse (note to self: adjust my Eclipse templates to remove that and a few other things!). I didn't spend too much time on unit testing purely because I wasn't sure still if this was the right place to commit this component, now that I am on the right track I will spend some time to implement those properly. Last but not least, org.apache.commons.functor.summarizer.TimedSummarizer#MAIN_TIMER is static in order to avoid creating a new one for each instance – but you are right, it can create a memory leak. I will actually spend some time on it and probably provide a factory for creating instances of the TimedSummarizer class which will allow for the instance to use it's own Timer or a shared one; this is so "power users" can use the shared one and minimize the memory and threading footprint, while still allowing for Joe Average to avoid the memory leakage by using a per-instance Timer . Do you think that would be a good idea?
        Hide
        Simone Tripodi added a comment -

        Hi Liviu,
        thanks for your contribution! some suggestions in order to apply the patch:

        • Instead of adding new interfaces, I suggest you reusing existing functor APIs, i.e. if I were you, I would modify the org.apache.commons.functor.aggregate.Aggregator<T> interface in order to inherit from org.apache.commons.functor.UnaryProcedure<A>; take a look at the existing codebase, don't just add new stuff!
        • The Apache header is missing in every class you contributed, it must be included in order to apply the patch;
        • Please remove the @author tags, committers/contributors are mentioned in the POM (ignore existing tags) - add yourself in the POM in the contributors list;
        • org.apache.commons.functor.summarizer.TimedSummarizer could inherit from org.apache.commons.functor.BinaryFunction;
        • I didn't understand why the org.apache.commons.functor.summarizer.TimedSummarizer#MAIN_TIMER is static, it IIUC could cause some leaks.
        • Test cases must be implement as valid JUnit tests executed by the surefire plugin, AFAIK usage of static main() methods as tests is not encouraged.

        Many thanks in advance for your effort, looking forward for the next patch!
        *

        Show
        Simone Tripodi added a comment - Hi Liviu, thanks for your contribution! some suggestions in order to apply the patch: Instead of adding new interfaces, I suggest you reusing existing functor APIs, i.e. if I were you, I would modify the org.apache.commons.functor.aggregate.Aggregator<T> interface in order to inherit from org.apache.commons.functor.UnaryProcedure<A> ; take a look at the existing codebase, don't just add new stuff! The Apache header is missing in every class you contributed, it must be included in order to apply the patch; Please remove the @author tags, committers/contributors are mentioned in the POM (ignore existing tags) - add yourself in the POM in the contributors list; org.apache.commons.functor.summarizer.TimedSummarizer could inherit from org.apache.commons.functor.BinaryFunction ; I didn't understand why the org.apache.commons.functor.summarizer.TimedSummarizer#MAIN_TIMER is static, it IIUC could cause some leaks. Test cases must be implement as valid JUnit tests executed by the surefire plugin, AFAIK usage of static main() methods as tests is not encouraged. Many thanks in advance for your effort, looking forward for the next patch! *
        Hide
        Liviu Tudor added a comment -

        Diff taken against trunk on 29/Aug/2011 (about midnight in UK

        Show
        Liviu Tudor added a comment - Diff taken against trunk on 29/Aug/2011 (about midnight in UK

          People

          • Assignee:
            Simone Tripodi
            Reporter:
            Liviu Tudor
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development