Developers of Sympa

[IKEDA Soji <address@concealed>]

Guillaume and all,

On Mon, 19 Aug 2013 14:36:19 +0200
Guillaume Rousse <address@concealed> wrote:

> Le 12/08/2013 15:23, IKEDA Soji a écrit :
> > On Mon, 12 Aug 2013 13:25:42 +0200
> > Guillaume Rousse <address@concealed> wrote:
> >
> >> Le 12/08/2013 12:36, IKEDA Soji a écrit :
> >>>>> I'd like to propose as following:
> >>>>>
> >>>>> o Spool class would be handled in single style:
> >>>>>
> >>>>>        my $spool = Sympa::Spool::Digest->new();
> >>>>>        my $spool = Sympa::Spool::Incoming->new();
> >>>>>        my $spool = Sympa::Spool::Task->new();
> >>>>>        my $spool = Sympa::Spool::Archiving->new();
> >>>> 'Task' and 'Digest' are nouns, whereas 'Archiving' and 'Incoming' are
> >>>> not, this is not consistent. I guess incoming spool is meant to store
> >>>> messages, so that should rather be:
> >>>> my $incoming = Sympa::Spool::Message->new();
> >>>
> >>> Please give appropriate names.  "Incoming" spool contains messages
> >>> sent from outer world.  "Archiving" spool contains messages to be
> >>> added to archives.
> >> You're confusing instance and classes names here, and I'm precisely
> >> trying to give classes appropriate names, according to what they store.
> >> If both incoming and archiving spool contains messages, they should be
> >> instances of the same Sympa::Spool::Message class, ie:
> >> my $digest    = Sympa::Spool::Digest->new(...);
> >> my $task      = Sympa::Spool::Task->new(...);
> >> my $incoming  = Sympa::Spool::Message->new(...);
> >> my $archiving = Sympa::Spool::Message->new(...);
> >>
> >> That is a coherent naming scheme.
> >
> > If that scheme did not include simplicity, I meant:
> >
> > my $spool = Sympa::Spool::Messages_to_be_Compiled_into_Digest->new();
> > my $spool = Sympa::Spool::Messages_Sent_from_Outer_World->new();
> > my $spool = Sympa::Spool::Tasks_Waiting_Execution->new();
> > my $spool = Sympa::Spool::Messages_to_be_Added_to_Archives->new();
> >
> > Would you please give appropriate names?
> I don't get your point here. Why would you need three different classes 
> to manage three different message spools ? As as spool purpose is just 
> to ensure persistancy by serialising data, that's just overkill for me.

Because it is one of possible options we may adopt.

Preparing separate subclasses by functions specific to each spools
can help readability (or maybe it can not: I have not decided by
now).


> [..]
> > You suggested:
> > | It would be clearer to enumerate the needed classes, and to define their
> > | exact relationships. For instance, how will you manage backends ? Yet
> > | another class hierarchy ?
> >
> > So I rewrote examples in top half of this post to suit to your
> > suggestion.  Translation: "$incoming is instance of Sympa::Spool
> > class using 'msg' domain of the storage implemented by
> > Sympa::Spool::Backend::SQL class and manipulates its contents as
> > instances of Sympa::Message class."
> You're giving an usage example, whereas my suggestion was to give an 
> exhaustive overview of the list of classes involved by your
> proposal. For instance, using the nomenclature I used in last month 
> discussion:
> 
> * Scenario B1:
> Sympa::Spool          abstract spool
> |- Sympa::Spool::SQL  SQL-based spool spool
> |- Sympa::Spool::File files-based spool
> 
> One single class hierarchy, for storage backends, and another class for 
> spool themselves.
> 
> * Scenario C1:
> Sympa::Spool                      abstract spool
> |- Sympa::Spool::Message          abstract message spool
> |  |- Sympa::Spool::Message::File file-based message spool
> |  |- Sympa::Spool::Message::SQL  SQL-based message spool
> |- Sympa::Spool::Task             abstract task spool
>     |- Sympa::Spool::Task::File    file-based task spool
>     |- Sympa::Spool::Task::SQL     SQL-based task spool
> 
> One single class hierarchy for all classes, with content as most 
> important sorting criteria.
> 
> * Scenario C2:
> Sympa::Spool                      abstract spool
> |- Sympa::Spool::File             abstract file spool
> |  |- Sympa::Spool::File::Message file-based message spool
> |  |- Sympa::Spool::File::Task    file-based task spool
> |- Sympa::Spool::SQL              abstract SQL spool
>     |- Sympa::Spool::SQL::Message  SQL-based message spool
>     |- Sympa::Spool::SQL::Task     SQL-based task spool
> 
> The alternative scenario to the previous, with storage as most important 
> sorting criteria.
> 
> * Scenario C3:
> Sympa::Spool                      spool class
> Sympa::Spool::Content             abstract spool stored content
> |- Sympa::Spool::Content::Message message stored content
> |- Sympa::Spool::Content::Task    task stored spool
> Sympa::Spool::Backend             abstract spool storage backend
> |- Sympa::Spool::Backend::SQL     SQL-based storage backend
> |- Sympa::Spool::Backend::File    filesystem-based storage backend
> 
> That's a more complex scenario, using two different classes hierarchy, 
> one the for storage backend, one for the stored content, and a distinct 
> class for spool.
> 
> David had strong arguments for either scenario C1 or C2 (saving 
> content-specific metadata using specific backend requires specific 
> code), whereas I'm more inclined toward B1, for sake of simplicity, as I 
> think we can achieve the same result using fewer classes.
> 
> One you defined the entites you are going to manipulate, now you can 
> discuss API.
> 
> Is that clearer ?

My snippet seems to belong to C3, but in practice it may not be so
heavy to implement.

- Content classes have (besides various features specific to each
  content type) small set of common interfaces to interact with
  spool class: serializing and deserializing.

  Such interface _may_ be defined by abstract class such as
  Sympa::Spool::Content above, or it _may_ be simply done by
  a definite set of methods such as "serialize()" and "new()".

  So, on this part, subclassing seems not to be essential for me.

- Backend classes provide feature to save serialized data into
  physical storage.

  They _shall_ be subclasses of single base class to provide
  common API for spool.


> >>>>> o Configuration parameters for (2) above would be defined for
> >>>>>      each spool.  Current possible values are "sql" and "filesystem".
> >>>>>
> >>>>>       digest_spool_storage
> >>>>>       incoming_spool_storage
> >>>>>       task_spool_storage
> >>>>>       ...
> >>>>>
> >>>>> Any comments?
> >>>> Is it really needed to mix spool storage type ? What about first 
> >>>> getting
> >>>> a simple, but working implementation, before reaching into unproven
> >>>> complexity ?
> >>>
> >>> As I remember ---
> >>> - In sympa-6.1, filesystem-based spool were used for all spools
> >>>     except for bulk sending.
> >>> - Then in sympa-6.2-branch, there was an attempt to switch almost
> >>>     all spools to SQL-based.
> >>> - However, this plan seems to have been partially changed (I don't
> >>>     excactly know why).
> >> It didn't supported the load, AFAIK. And the current plan was to allow
> >> both types, with a single configuration switch (ie, all file-based, or
> >> all sql-based spools).
> >
> > I quitely presumed switching should be possible by each spool.
> >
> > Actually, there may not be obvious consensus on this plan.
> There isn't any clear specification here, hence the confusion. For me, 
> the 7.0 plan was:
> - support for both filesystem and SQL spool
> - a single configuration directive to select which backend to use globally
> 
> That's enough complexity for me already, given our current workload.

I see.  I vote for single configuration directive.


-- 
IKEDA Soji <address@concealed>