Developers of Sympa

[IKEDA Soji <address@concealed>]

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?


> >>> o Each class is a subclass of common abstract class (i.e.
> >>>     Sympa::Spool) and has following variations:
> >>>
> >>>      (1) Content object to handle (message digest, Message, Task, ...),
> >>>        defined by each class itself.
> >>>      (2) Physical backend (SQL, filesystem, ...) defined by 
> >>> configuration
> >>>        option.
> >>>      (3) Domain in the backend (on SQL, value of name_spool column;
> >>>        on filesystem, name of subdirectories: "digest", "msg", "task",
> >>>        ...) defined by each class itself.
> >> 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 ?
> >
> > Of course, another class will be used.  I have not yet understood why
> > you are rejecting "two class hierarchies" idea.
> >
> >> And I still object having spool classes hardcoding their settings, which
> >> tends to multiply artificially the number of classes needed. If you
> >> hardcode the fact than spool 'X' uses domain 'X' in a Sympa::Spool::X
> >> class, than you'll need as much classes as you have spools. For me,
> >> those settings should be passed at instanciation time by calling code, 
> >> ie:
> >>
> >> my $incoming = Sympa::Spool::Message->new(
> >>       storage => $configuration->get_param('spool_storage'),
> >>       domain  => 'incoming',
> >>       name    => 'incoming'
> >> );
> >
> > "name" looks duplicate of "domain".  So if I wrote in similar style:
> >
> > my $incoming = Sympa::Spool->new(
> >      generator => 'Sympa::Message',
> >      storage => 'Sympa::Spool::Backend::SQL', # taken from 
> > incoming_spool_storage
> >      domain  => 'msg',                        # name of spool for 
> > incoming messages.
> > );
> What it is supposed to return ?

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


I don't know which style is better.


<<sorry for omission>>

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


-- 
IKEDA Soji <address@concealed>