[prev in list] [next in list] [prev in thread] [next in thread] 

List:       bricolage-devel
Subject:    Re: [Bricolage-Devel] Spooler and Transport abstraction
From:       Joao Pedro Goncalves <joaop () co ! sapo ! pt>
Date:       2002-07-10 10:48:29
[Download RAW message or body]

On Tue, 2002-07-09 at 20:52, David Wheeler wrote:
> 
> Does the code assume that the spooling daemon is on the local server? What
> would it take to allow it to run on its own server? Would you need to add a
> couple of configuration parameters?

I've only tested Net::Spooler with unix sockets, but it probably works
as fine with tcp sockets. This would need of course some extra
configuration parameters (SPOOLER_SERVER SPOOLER_PORT).

> 
> In SpoolClient.pm, you don't need to include the package name in the
> exception messages. The exceptions contain stack traces, so it'd be
> redundant.

Ok.

> 
> In Spool.pm, I suggest you check $@ after loading each $pkg. Also, you can
> dump the print statements, or only print them if DEBUG is enabled or
> something.

Ok.

> 
> In Spool.pm, ProcessFile() isn't documented. Also, I should note at this
> point that Bricolage's method names adhere to the usual Perl semantics --
> e.g., lowercase method names with words separated by underscores. Unless,
> that is, some other module requires a method of a particular name.

That's the case here, ProcessFile is called by Net::Spooler, and
documented as such in Net::Spooler. I'll document that and what
it does in the next patch.


> 
> In Spool.pm, you have this line:
> 
>                $mail_obj->_set([qw(to from subject message)],
> 
> Of course, _set() is a private method. Please use the public methods
> instead. The same holds true for other places where you do this.

Sorry :>. I'll stick to the public stuff :).


> 
> Also, I think it might be best to use some other kind of logging than
> sending an email in the event of a failure -- especially since it could be
> email that failed! Sam, what kind of logging do you do in the SOAP stuff?
> bric_dist_mon uses Unix::Syslog.

Yeap, i was just emulating what Net::Spooler has documented but doesn't
do. 
Next patch uses Unix::Syslog.


> 
> In Spool.pm, you have this line:
> 
>             my $time = $@ ? (warn "Alert failed to send: $@\n", undef)[1]
>                 : db_date(undef, 1);
> 
> Where are you expecting $@ to have been set? Note that if it's C<eval {
> $alert_obj->send() };>, that it won't be there anymore, because the DBI
> method calls in between reset it. You'll need to copy $@ into a lexical
> before calling any other methods or functions.

Ok, i'll check on this.

> 
> I still think that the column in the contact table should be alert_class_id,
> rather than class__id.

Sorry, you had already mentioned this. I just forgot to change it.


> use constant SPOOL_DIR   =>  Bric::Util::Trans::FS->cat_dir(TEMP_DIR,
> "bricolage", "spool");
> 
> ...to maintain cross-platform compatibility.

Ok.

> 
> You should similarly use Bric::Util::Trans::FS->cat_dir to set UNIX_SOCKET,
> LOG_FILE, and PID_FILE.
> 
> -# Alert Settings. ALERT_FROM is the email address that will appear in the
> 'From'
> +# Alert Settings. ALERT_FROM is the email address that will appear in the
> 'From
> 
> I don't think you actually meant to eliminate the apostrophe (') from the
> end of that line, did you?

Nope.

> 
> ENABLE_JABBER_ALERT     = 1
> +JABBER_SERVER           = sim.sapo.pt
> +JABBER_PORT             = 5222
> +JABBER_USER             = joao
> +JABBER_PASSWORD         = sorvete
> 
> These should probably have some more generic defaults, no? Like jabber.org
> for the default server? Actually, I think that USER and PASSWORD should
> probably be empty by default; They'll throw an error if they're not filled
> in and they're uncommented. They should all be commented out by default, and
> ENABLE_JABBER_ALERT should be set to "Off" by default. Also, the
> documentation in Admin.pod and the code in Config.pm should reflect this,
> too.

Ouch. Those are 'real' values. I'll never review patches that sleepy
again. Me change password now :).

> 
> In Alert.pm, don't use File::Spec::Unix. Use Bric::Util::Trans::FS, instead.
> Always use that class for doing stuff with files and directories on the file
> system. If you need to do stuff that's Unix-specific (like URIs), then use
> the methods in the class for managing URIs, such as cat_uri().

Okey.


> I think I might like to see SpoolClient::queue() implemented as a method
> rather than a function. Not a big deal, though.

Yes, firstly i had it that way, but since each queue() request was
needing to open a new socket and closing it again, it semed more like a
function than a method with some persistency between calls.


> 
> In Jabber.pm, s/Cyclops/Bricolage/g; :o)

Sleepyhead..


> 
> Also, use the TEMP_DIR directive from Config.pm rather than a private one.
> 
> The documentation for unregister() has "register" in its heading. I don't
> think you need the "=over 4" above it, either.

Changing it.

> 
> Okay, now I respond to *your* notes:
  
> > - Bric::Util::Trans::SpoolClient has a single function,
> > queue_message($msg). It's used by Bric::Util::Alert.
> > 
> > - Logging of alerted users is now handled by Trans::Spooler,
> > when alerted_users and alert id is passed (Documented).
> > 
> > - Bric::Admin now includes documentation regarding starting
> > bric_spooler (in my lousy english of course).
> 
> I think you might need to update the documentation of Bric::Alerts, too.

Okey.

>  
> > Oh, one question:
> > 
> > bric_spooler is currently detaching from the terminal by using:
> > 
> > my $pid = fork;
> > exit if $pid;
> > 
> > Is this the best way? Are there any parts of bricolage that
> > do this?
> 
> Again, I'm not authority on forking, so I invite others to chime in here.
> But as far as I'm concerned, it looks fine.

Proc::Daemon rules!

> 
> From where I sit, I think that this is almost ready. If you would be so kind
> as to submit another patch addressing the issues I raise above, I'll apply
> it to my local install and take it for a spin.

Thanks for your comments David!

JP


 
 
-- 
João Pedro Gonçalves
SAPO.pt - PT Multimedia


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Two, two, TWO treats in one.
http://thinkgeek.com/sf
_______________________________________________
Bricolage-Devel mailing list
Bricolage-Devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bricolage-devel
[prev in list] [next in list] [prev in thread] [next in thread]