onegative has asked for the wisdom of the Perl Monks concerning the following question:
G'day most honorable Monks, I am asking for assistance in my approach to coding a writer into a MySQL database. As you can no doubt see from my coding I am sure you all would consider me a novice Perl coder and I would appreciate any critiques about my approach. The purpose of this code will be to sit on four or more servers where work is performed and therefore watch an active log from each server. I intend on running a File::Tail script to watch these logs and then process the entries into a single MySQL database, four tables. My concern is with the use of this script on four or more servers and the possible contention/locks on tables as different servers attempt to write their data. I am wondering are there ways to change my code to help reduce this situation or whether this is a valid concern in the first place. All these servers are a minimum of 4 processor/4G memory with very low loads. The stored data will then be used as a central advance log search tool via a PHP interface. Again, I welcome constructive criticism as it helps me learn and grow my skill with Perl. Thank you in Advance, Danny p.s. I forgot to mention the code does function as it has been tested on my development system. But short-cuts are always welcomed.
#!/usr/local/bin/perl -w
use strict;
use DBI;
use File::Tail;
# Create File::Tail object
my $file;
$file=File::Tail->new("/opt/app/superspy/tmp/commLogger.log");
my $line;
while (defined($line=$file->read))
{
chomp($line);
# print "$line\n";
# Create DB connection
my $db_handle = DBI->connect("dbi:mysql:database=P0TA6E01;host
+=myhost02.fcc.hide.com:3306;user=xxxxxxxx;password=xxxxxxxx") || die
+"Couldn't connect to database: $DBI::errstr\n";
if($line =~ /^AlertCmd~.*~.*~-reload.*/){next;}
if($line =~ /^AlertCmd~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)/){
my $tstamp = $1;
my $cmnd_id = $2;
my $cmnd = $3;
my $client_host = $4;
my $client_user = $5;
my $svr_host = $6;
$svr_host =~ s/\..*//g;
my $sql = "INSERT INTO ALERTS (TSTAMP,CMND_ID,CMND,CLI
+ENTHOST,CLIENTUSER,SVR_HOST) VALUES (\'$tstamp\',\'$cmnd_id\',\'$cmnd
+\',\'$client_host\',\'$client_user\',\'$svr_host\')\;\n";
my $statement = $db_handle->prepare($sql)
or die "Couldn't prepare query '$sql': $DBI::errst
+r\n";
$statement->execute()
or die "Couldn't execute query '$sql': $DBI::errst
+r\n";
}
if($line =~ /^SendFiltered~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)/){
my $tstamp = $1;
my $cmnd_id = $2;
my $alert_id = $3;
my $send_id = $4;
my $state = $5;
my $svr_host = $6;
$svr_host =~ s/\..*//g;
my $sql = "INSERT INTO FILTERED (TSTAMP,CMND_ID,ALERT_
+ID,SEND_ID,STATE,SVR_HOST) VALUES (\'$tstamp\',\'$cmnd_id\',\'$alert_
+id\',\'$send_id\',\'$state\',\'$svr_host\')\;\n";
my $statement = $db_handle->prepare($sql)
or die "Couldn't prepare query '$sql': $DBI::errst
+r\n";
$statement->execute()
or die "Couldn't execute query '$sql': $DBI::errst
+r\n";
}
if($line =~ /^SendOffDuty~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)/){
my $tstamp = $1;
my $cmnd_id = $2;
my $alert_id = $3;
my $send_id = $4;
my $state = $5;
my $svr_host = $6;
$svr_host =~ s/\..*//g;
my $sql = "INSERT INTO FILTERED (TSTAMP,CMND_ID,ALERT_
+ID,SEND_ID,STATE,SVR_HOST) VALUES (\'$tstamp\',\'$cmnd_id\',\'$alert_
+id\',\'$send_id\',\'$state\',\'$svr_host\')\;\n";
my $statement = $db_handle->prepare($sql)
or die "Couldn't prepare query '$sql': $DBI::errst
+r\n";
$statement->execute()
or die "Couldn't execute query '$sql': $DBI::errst
+r\n";
}
if($line =~ /^SendStarted~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)~(
+.*)~(.*)~(.*)~(.*)/){
my $tstamp = $1;
my $cmnd_id = $2;
my $alert_id = $3;
my $send_id = $4;
my $user = $5;
my $destination = $6;
my $pin = $7;
my $to = $8;
my $message = $9;
my $state = $10;
my $svr_host = $11;
$svr_host =~ s/\..*//g;
my $address;
if($pin eq ""){$address = $to;}else{$address = $pin;}
my $sql = "INSERT INTO SENDS (START_TIME,CMND_ID,ALERT
+_ID,SEND_ID,USER,DESTINATION,ADDRESS,MESSAGE,STATE,SVR_HOST,ACTIVE) V
+ALUES (\'$tstamp\',\'$cmnd_id\',\'$alert_id\',\'$send_id\',\'$user\',
+\'$destination\',\'$address\',\'$message\',\'$state\',\'$svr_host\',\
+'TRUE\')\;\n";
my $statement = $db_handle->prepare($sql)
or die "Couldn't prepare query '$sql': $DBI::errst
+r\n";
$statement->execute()
or die "Couldn't execute query '$sql': $DBI::errst
+r\n";
}
if($line =~ /^SendCompleted~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)/){
my $tstamp = $1;
my $cmnd_id = $2;
my $alert_id = $3;
my $send_id = $4;
my $state = $5;
my $svr_host = $6;
$svr_host =~ s/\..*//g;
my $sql = "UPDATE SENDS SET END_TIME=\'$tstamp\',STATE
+=\'$state\',ACTIVE=\'FALSE\' WHERE CMND_ID=\'$cmnd_id\' AND ALERT_ID=
+\'$alert_id\' AND SEND_ID=\'$send_id\' AND SVR_HOST=\'$svr_host\'\;\n
+";
my $statement = $db_handle->prepare($sql)
or die "Couldn't prepare query '$sql': $DBI::errst
+r\n";
$statement->execute()
or die "Couldn't execute query '$sql': $DBI::errst
+r\n";
}
if($line =~ /^SendCleared~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)/){
my $tstamp = $1;
my $cmnd_id = $2;
my $alert_id = $3;
my $send_id = $4;
my $state = $5;
my $svr_host = $6;
$svr_host =~ s/\..*//g;
my $sql = "UPDATE SENDS SET END_TIME=\'$tstamp\',STATE
+=\'$state\',ACTIVE=\'FALSE\' WHERE CMND_ID=\'$cmnd_id\' AND ALERT_ID=
+\'$alert_id\' AND SEND_ID=\'$send_id\' AND SVR_HOST=\'$svr_host\'\;\n
+";
my $statement = $db_handle->prepare($sql)
or die "Couldn't prepare query '$sql': $DBI::errst
+r\n";
$statement->execute()
or die "Couldn't execute query '$sql': $DBI::errst
+r\n";
}
if($line =~ /^AlertError~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)~(.
+*)/){
my $tstamp = $1;
my $cmnd_id = $2;
my $alert_id = $3;
my $send_id = $4;
my $destination = $5;
my $message = $6;
my $state = $7;
my $svr_host = $8;
$svr_host =~ s/\..*//g;
my $sql = "INSERT INTO ERRORS (TSTAMP,CMND_ID,ALERT_ID
+,SEND_ID,DESTINATION,MESSAGE,STATE,SVR_HOST) VALUES (\'$tstamp\',\'$c
+mnd_id\',\'$alert_id\',\'$send_id\',\'$destination\',\'$message\',\'$
+state\',\'$svr_host\')\;\n";
my $statement = $db_handle->prepare($sql)
or die "Couldn't prepare query '$sql': $DBI::errst
+r\n";
$statement->execute()
or die "Couldn't execute query '$sql': $DBI::errst
+r\n";
}
$statement->finish();
$db_handle->disconnect();
}
Re: Need Critique (Good or Bad)
by moritz (Cardinal) on Feb 05, 2008 at 16:08 UTC
|
Just a few things from skimming over your code:
Use Placeholders!.
Instead of
my $sql = "INSERT INTO ERRORS (TSTAMP,CMND_ID,ALERT_ID,SEND_ID,DESTIN
+ATION,MESSAGE,STATE,SVR_HOST) VALUES (\'$tstamp\',\'$cmnd_id\',\'$ale
+rt_id\',\'$send
+_id\',\'$destination\',\'$message\',\'$state\',\'$svr_host\')\;\n";
my $statement = $db_handle->prepare($sql)
or die "Couldn't prepare query '$sql': $DBI::errst
+r\n";
$statement->execute()
or die "Couldn't execute query '$sql': $DBI::errst
+r\n";
write
my $sql = qq{"INSERT INTO ERRORS (TSTAMP,CMND_ID,ALERT_ID,SEND_ID,DEST
+INATION,MESSAGE,STATE,SVR_HOST) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?);
my $statement = $dbh->prepare($sql);
$statement->execute($tstamp, $cmnd_id, ...);
Also make sure to pass the {RaiseError =>1} argument to DBI->connect, then you don't need the custom error handlers.
Even more important: you can prepare the statements once outside the loop, and then call execute them inside the loops as many times as you need them. That way you'll gain much performance (if you have many iterations).
Instead of
if($line =~ /^SendCleared~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)/){
my $tstamp = $1;
my $cmnd_id = $2;
my $alert_id = $3;
my $send_id = $4;
my $state = $5;
my $svr_host = $6;
you can first check if the line starts with SendCleared and then use
my ($tstamp, $cmnd_id, ...) = split m/~/, $line;
There are actually even better ways to handle the dispatch with a hash: you can store these sql queries in a has with keys SendOffDuty, AlertCmd and so on, and then do something like that:
# assume %q contains the quries
my ($query, @args) = split $line, '~';
$q{$query}->execute(@args); # assuming the log files are well formed
| [reply] [d/l] [select] |
Re: Need Critique (Good or Bad)
by thparkth (Beadle) on Feb 05, 2008 at 16:38 UTC
|
There's always more than one way to do it, but I think you're working harder here than you have to.
All your
if ($line =~...
...blocks are very similar.
I would probably suggest not using the heavy regular expressions as you are doing, but rather using the split operator. And I wouldn't treat the first token in the line any differently from the rest. I'd also be lazier than you about picking out each field by name. Something like this:
my ($type,@tokens)=split(/\~/,$line);
if ($type eq "SendFiltered")
{
my ($tstamp,$cmnd_id,$alert_id,$send_id,$state,$svr_host)
= @tokens[0-5];
my $sql = "INSERT ...
}
elsif ($type eq "AlertError")
{
...
}
However even that isn't really sufficiently lazy. The SQL is still there, and it's still ugly. (This isn't your fault - SQL is just plain ugly.)
It seems to me that since you're not doing anything at all with all of these values except sticking them in the SQL statements, you don't actually need to bother naming them at all. Really all you need to know for each "first token" type is what the SQL statement should be, and which column numbers to plug in.
With that in mind, you could do something like this:
# %statements is an array of prepared SQL statement objects ready to h
+ave values plugged in
%statements=(
"AlertError"=>$dbh->prepare("INSERT INTO ERRORS (TSTAMP,CMND_ID,ALER
+T_ID,SEND_ID,DESTINATION,MESSAGE,STATE,SVR_HOST) VALUES (?,?,?,?,?,?,
+?,?)"),
"SendCleared"=>$dbh->prepare("UPDATE SENDS..."),
"...
);
# %columns is an array of column position indicators corresponding to
+the %statements above
%columns=(
"AlertError"=>[0,1,2,3,4,5],
"SendCleared"=>[...
);
Then your actual executable code becomes quite easy...
while ($line=...)
{
my ($type,@tokens)=split(/\~/,$line);
$statements{$type}->execute(@tokens[@{$columns{$type}}]);
}
There are various other ways you could organise the data, including using OOP. The key point is that the SQL statements and the column mappings really are data, not code, from the point of view of your Perl program. | [reply] [d/l] [select] |
Re: Need Critique (Good or Bad)
by apl (Monsignor) on Feb 05, 2008 at 16:23 UTC
|
In addition to all of the above, I'd suggest taking all of your $line testing and putting it in one procedure. This provides two benefits:
| [reply] [d/l] |
Re: Need Critique (Good or Bad)
by holli (Abbot) on Feb 05, 2008 at 22:57 UTC
|
I have refactored your code. it's untested (but should run) and shows some of the principles mentioned in the other replies, including the use of a dispatch table. feel free to ask if you don't understand something.
#!/usr/local/bin/perl -w
use strict;
use DBI;
use File::Tail;
# query subs, fetch and name the parameters and call the prepared stat
+ement
sub insert_alerts
{
my ($sth, $tstamp, $cmnd_id, $cmnd , $client_host, $client_user, $
+svr_host, $svr_host) = @_;
$svr_host =~ s/\..*//g;
$sth->execute( $tstamp, $cmnd_id, $cmnd , $client_host, $client_us
+er, $svr_host, $svr_host );
return $DBI::errstr ? 0 : 1;
}
sub insert_filters
{
my ($sth, $tstamp, $cmnd_id, $cmnd , $client_host, $client_user, $
+svr_host, $svr_host) = @_;
$svr_host =~ s/\..*//g;
$sth->execute( $tstamp, $cmnd_id, $cmnd , $client_host, $client_us
+er, $svr_host, $svr_host );
return $DBI::errstr ? 0 : 1;
}
sub insert_sends
{
my ($sth, $tstamp, $cmnd_id, $alert_id, $send_id, $user, $destinat
+ion, $pin, $to, $message, $state, $svr_host) = @_;
$svr_host =~ s/\..*//g;
$sth->execute( $tstamp, $cmnd_id, $alert_id, $send_id, $user, $des
+tination, $pin||$to, $message, $state, $svr_host, 'TRUE' );
return $DBI::errstr ? 0 : 1;
}
sub update_sends
{
my ($sth, $tstamp, $cmnd_id, $alert_id, $send_id, $state, $svr_hos
+t = $6) = @_;
$svr_host =~ s/\..*//g;
$sth->execute( $tstamp, $state, $cmnd_id, $alert_id, $send_it, $sv
+r_host );
return $DBI::errstr ? 0 : 1;
}
sub insert_errors
{
my ($sth, $tstamp, $cmnd_id, $cmnd , $client_host, $client_user, $
+svr_host, $svr_host) = @_;
$svr_host =~ s/\..*//g;
$sth->execute( $tstamp, $cmnd_id, $cmnd , $client_host, $client_us
+er, $svr_host, $svr_host );
return $DBI::errstr ? 0 : 1;
}
# Create File::Tail object
my $file = File::Tail->new("/opt/app/superspy/tmp/commLogger.log");
# Create DB connection
my $dbh = DBI->connect("dbi:mysql:database=P0TA6E01;host=myhost02.fcc.
+hide.com:3306;user=xxxxxxxx;password=xxxxxxxx") || die "Couldn't conn
+ect to database: $DBI::errstr\n";
# dispatch table and prepared statenents for the logfile entries
# this whill be faster than preparing the query for each entry
my %dispatch =
(
AlertCmd => [ \&insert_alerts, $dbh->prepare('INSERT INTO AL
+ERTS (TSTAMP,CMND_ID,CMND,CLIENTHOST,CLIENTUSER,SVR_HOST) VALUES (?,?
+,?,?,?,?);' ],
SendFiltered => [ \&insert_filters, $dbh->prepare('INSERT INTO FI
+LTERS (TSTAMP,CMND_ID,CMND,CLIENTHOST,CLIENTUSER,SVR_HOST) VALUES (?,
+?,?,?,?,?);' ],
SendOffDuty => [ \&insert_filters, $dbh->prepare('INSERT INTO FI
+LTERS (TSTAMP,CMND_ID,CMND,CLIENTHOST,CLIENTUSER,SVR_HOST) VALUES (?,
+?,?,?,?,?);' ],
SendStarted => [ \&insert_sends, $dbh->prepare('INSERT INTO SE
+NDS (START_TIME,CMND_ID,ALERT_ID,SEND_ID,USER,DESTINATION,ADDRESS,MES
+SAGE,STATE,SVR_HOST,ACTIVE) VALUES (?,?,?,?,?,?,?,?,?,?,?)' ],
SendCompleted => [ \&update_sends, $dbh->prepare('UPDATE SENDS S
+ET END_TIME=?, STATE=?,ACTIVE=FALSE WHERE CMND_ID=? AND ALERT_ID=? AN
+D SEND_ID=? AND SVR_HOST=?;' ],
SendCleared => [ \&update_sends, $dbh->prepare('UPDATE SENDS S
+ET END_TIME=?, STATE=?,ACTIVE=FALSE WHERE CMND_ID=? AND ALERT_ID=? AN
+D SEND_ID=? AND SVR_HOST=?;' ],
AlertError => [ \&insert_errors, $dbh->prepare('INSERT INTO ER
+RORS (TSTAMP,CMND_ID,ALERT_ID,SEND_ID,DESTINATION,MESSAGE,STATE,SVR_H
+OST) VALUES (?,?,?,?,?,?,?,?)' ],
);
# read linewise
while (my $line = $file->read))
{
chomp($line);
# skipt reload
next
if $line =~ /^AlertCmd~.*~.*~-reload.*/;
# check for interesting entries, save keyword and the rest in $1 a
+nd $2
if ( $line =~ /^(AlertCmd|SendFiltered|SendOffDuty|SendStarted|Sen
+dCompleted|SendCleared|AlertError)~(.+)/ )
{
# the callback and the prepared statement from the dispatch ta
+ble that are appropriate for the entry
my ($function, $sth) = @{ $dispatch->{$1} };
# call the function and show error if something went wrong
unless ( $function->( $sth, split /~/, $2 ) )
{
print "Error ($DBI::errstr) while processing this line:\n$
+line\n\n";
}
}
}
| [reply] [d/l] |
|
Hey Holli, A picture is worth a thousand words...thanks for the refactor it explains the use of these concepts and I follwed all parts of the code. I will definately use these concepts in the future... Many Thanks to Everyone, Danny
| [reply] |
|
|