[patch] Perl Cache::Memcached, stats() and tests

Ronald J Kimball rkimball+memcached at pangeamedia.com
Tue Jul 3 15:19:15 UTC 2007


I don't think updating the tests was completely unrelated in this case, 
because I was adding tests for my fix to stats().

Anyway, I've separated this into two separate patches.  The first patch 
fixes stats() in Memcached.pm.  The second patch updates the test files. 
use.t is renamed to 01_use.t.  all.t is renamed to 02_keys.t, and now 
includes tests for delete().  03_stats.t is a new test file, containing 
the stats() tests that were in all.t and also some new tests for 
stats().  (Apply cache_memcached_stats.patch first, then 
cache_memcached_tests.patch.)

Does that work?


Ronald


Brad Fitzpatrick wrote:
> Ronald,
> 
> Thanks!
> 
> But one logical change per patch, please.  Things like "I also took this
> opportunity to change unrelated thing $X" tend to get your patch ignored
> (not just on this list, but most projects) because it's too hard to deal
> with/read/etc.
> 
> See:
>     http://danga.com/commit.bml
> 
> If you agree to those general rules and send me the htdigest line
> described at bottom, you can commit these patches one at a time by
> yourself.  Or, feel free to resend and one of us on the list can do it.
> 
> - Brad
> 
> 
> On Tue, 19 Jun 2007, Ronald J Kimball wrote:
> 
>> After upgrading to Cache::Memcached 1.22, I still get warnings from
>> calling stats() after a server has been restarted:
>>
>> Use of uninitialized value in addition (+) at
>> /usr/share/perl5/Cache/Memcached.pm line 876.
>> Use of uninitialized value in addition (+) at
>> /usr/share/perl5/Cache/Memcached.pm line 885.
>>
>> This is because, although stats() skips the host if the socket is dead,
>> it still tallies that host's stats for the total stats.
>>
>> Additionally, stats() tallies each host's misc stats for the total
>> stats, even when misc stats were not requested (e.g.
>> $memd->stats('malloc')).  This caused warnings and put spurious 0 values
>> in the stats totals.
>>
>>
>> The attached patch fixes these issues, by moving the calculation of
>> total stats into the processing loop.
>>
>>
>> I also took this opportunity to update the tests for Cache::Memcached:
>> I renamed use.t to 01_use.t.
>> I renamed all.t to 02_keys.t and added tests for delete().
>> I created 03_stats.t to test the results of stats().
>> The numbers ensure that the test files are run in the appropriate order.
>>
>>
>> Ronald
>>
>>

-------------- next part --------------
diff -rN -u Cache-Memcached-1.22.orig/lib/Cache/Memcached.pm Cache-Memcached-1.22/lib/Cache/Memcached.pm
--- Cache-Memcached-1.22.orig/lib/Cache/Memcached.pm	2007-06-18 13:51:02.000000000 -0400
+++ Cache-Memcached-1.22/lib/Cache/Memcached.pm	2007-06-19 17:18:00.000000000 -0400
@@ -820,9 +820,15 @@
         $stats_hr->{'self'} = \%{ $self->{'stats'} };
     }
 
+    my %misc_keys = map { $_ => 1 }
+      qw/ bytes bytes_read bytes_written
+          cmd_get cmd_set connection_structures curr_items
+          get_hits get_misses
+          total_connections total_items
+        /;
+
     # Now handle the other types, passing each type to each host server.
     my @hosts = @{$self->{'buckets'}};
-    my %malloc_keys = ( );
   HOST: foreach my $host (@hosts) {
         my $sock = $self->sock_to_host($host);
       TYPE: foreach my $typename (grep !/^self$/, @$types) {
@@ -852,7 +858,10 @@
                     if ($key) {
                         $stats_hr->{'hosts'}{$host}{$typename}{$key} = $value;
                     }
-                    $malloc_keys{$key} = 1 if $key && $typename eq 'malloc';
+                    $stats_hr->{'total'}{$key} += $value
+                        if $typename eq 'misc' && $key && $misc_keys{$key};
+                    $stats_hr->{'total'}{"malloc_$key"} += $value
+                        if $typename eq 'malloc' && $key;
                 }
             } else {
                 # This stat is not key-value so just pull it
@@ -864,29 +873,6 @@
         }
     }
 
-    # Now get the sum total of applicable values.  First the misc values.
-    foreach my $stat (qw(
-        bytes bytes_read bytes_written
-        cmd_get cmd_set connection_structures curr_items
-        get_hits get_misses
-        total_connections total_items
-        )) {
-        $stats_hr->{'total'}{$stat} = 0;
-        foreach my $host (@hosts) {
-            $stats_hr->{'total'}{$stat} +=
-                $stats_hr->{'hosts'}{$host}{'misc'}{$stat};
-        }
-    }
-
-    # Then all the malloc values, if any.
-    foreach my $malloc_stat (keys %malloc_keys) {
-        $stats_hr->{'total'}{"malloc_$malloc_stat"} = 0;
-        foreach my $host (@hosts) {
-            $stats_hr->{'total'}{"malloc_$malloc_stat"} +=
-                $stats_hr->{'hosts'}{$host}{'malloc'}{$malloc_stat};
-        }
-    }
-
     return $stats_hr;
 }
 
-------------- next part --------------
diff -rN -u Cache-Memcached-1.22.orig/MANIFEST Cache-Memcached-1.22/MANIFEST
--- Cache-Memcached-1.22.orig/MANIFEST	2007-04-17 02:44:09.000000000 -0400
+++ Cache-Memcached-1.22/MANIFEST	2007-06-19 17:22:46.000000000 -0400
@@ -6,6 +6,7 @@
 MANIFEST
 MANIFEST.SKIP
 TODO
-t/use.t
-t/all.t
+t/01_use.t
+t/02_keys.t
+t/03_stats.t
 META.yml                                 Module meta-data (added by MakeMaker)
diff -rN -u Cache-Memcached-1.22.orig/t/01_use.t Cache-Memcached-1.22/t/01_use.t
--- Cache-Memcached-1.22.orig/t/01_use.t	1969-12-31 19:00:00.000000000 -0500
+++ Cache-Memcached-1.22/t/01_use.t	2006-03-23 13:39:36.000000000 -0500
@@ -0,0 +1,7 @@
+#!/usr/bin/env perl -w
+use strict;
+use Test;
+BEGIN { plan tests => 1 }
+
+use Cache::Memcached; ok(1);
+exit;
diff -rN -u Cache-Memcached-1.22.orig/t/02_keys.t Cache-Memcached-1.22/t/02_keys.t
--- Cache-Memcached-1.22.orig/t/02_keys.t	1969-12-31 19:00:00.000000000 -0500
+++ Cache-Memcached-1.22/t/02_keys.t	2007-06-19 16:56:26.000000000 -0400
@@ -0,0 +1,35 @@
+# -*-perl-*-
+
+use strict;
+use Test::More;
+use Cache::Memcached;
+use IO::Socket::INET;
+
+my $testaddr = "127.0.0.1:11211";
+my $msock = IO::Socket::INET->new(PeerAddr => $testaddr,
+                                  Timeout  => 3);
+if ($msock) {
+    plan tests => 10;
+} else {
+    plan skip_all => "No memcached instance running at $testaddr\n";
+    exit 0;
+}
+
+my $memd = Cache::Memcached->new({
+    servers   => [ $testaddr ],
+    namespace => "Cache::Memcached::t/$$/" . (time() % 100) . "/",
+});
+
+ok($memd->set("key1", "val1"), "set key1 as val1");
+
+is($memd->get("key1"), "val1", "get key1 is val1");
+ok(! $memd->add("key1", "val-replace"), "add key1 failed");
+ok($memd->add("key2", "val2"), "add key2 as val2");
+is($memd->get("key2"), "val2", "get key2 is val2");
+
+ok($memd->replace("key2", "val-replace"), "replace key2 as val-replace");
+is($memd->get("key2"), "val-replace", "get key2 is val-replace");
+ok(! $memd->replace("key-noexist", "bogus"), "replace key-noexist failed");
+
+ok($memd->delete("key1"), "delete key1");
+ok(! $memd->get("key1"), "get key1 failed");
diff -rN -u Cache-Memcached-1.22.orig/t/03_stats.t Cache-Memcached-1.22/t/03_stats.t
--- Cache-Memcached-1.22.orig/t/03_stats.t	1969-12-31 19:00:00.000000000 -0500
+++ Cache-Memcached-1.22/t/03_stats.t	2007-06-19 16:58:13.000000000 -0400
@@ -0,0 +1,78 @@
+# -*-perl-*-
+
+use strict;
+use Test::More;
+use Cache::Memcached;
+use IO::Socket::INET;
+
+my $testaddr = "127.0.0.1:11211";
+my $msock = IO::Socket::INET->new(PeerAddr => $testaddr,
+                                  Timeout  => 3);
+
+my @misc_stats_keys = qw/ bytes bytes_read bytes_written
+                          cmd_get cmd_set connection_structures curr_items
+                          get_hits get_misses
+                          total_connections total_items
+                         /;
+
+if ($msock) {
+    plan tests => 24 + scalar(@misc_stats_keys);
+} else {
+    plan skip_all => "No memcached instance running at $testaddr\n";
+    exit 0;
+}
+
+my $memd = Cache::Memcached->new({
+    servers   => [ $testaddr ],
+    namespace => "Cache::Memcached::t/$$/" . (time() % 100) . "/",
+});
+
+my $misc_stats = $memd->stats('misc');
+ok($misc_stats, 'got misc stats');
+isa_ok($misc_stats, 'HASH', 'misc stats');
+isa_ok($misc_stats->{'total'}, 'HASH', 'misc stats total');
+isa_ok($misc_stats->{'hosts'}, 'HASH', 'misc stats hosts');
+isa_ok($misc_stats->{'hosts'}{$testaddr}, 'HASH',
+       "misc stats hosts $testaddr");
+
+foreach my $stat_key (@misc_stats_keys) {
+    ok(exists $misc_stats->{'total'}{$stat_key},
+       "misc stats total contains $stat_key");
+    ok(exists $misc_stats->{'hosts'}{$testaddr}{'misc'}{$stat_key},
+       "misc stats hosts $testaddr misc contains $stat_key");
+}
+
+my $got_malloc = 0;
+foreach my $stat_key (keys %{$misc_stats->{'total'}}) {
+    if ($stat_key =~ /^malloc/) {
+        $got_malloc = 1;
+        last;
+    }
+}
+ok(! $got_malloc, 'no malloc stats in misc stats');
+
+my $malloc_stats = $memd->stats('malloc');
+ok($malloc_stats, 'got malloc stats');
+isa_ok($malloc_stats, 'HASH', 'malloc stats');
+isa_ok($malloc_stats->{'total'}, 'HASH', 'malloc stats total');
+isa_ok($misc_stats->{'hosts'}, 'HASH', 'malloc stats hosts');
+isa_ok($misc_stats->{'hosts'}{$testaddr}, 'HASH',
+       "malloc stats host $testaddr");
+
+$got_malloc = 0;
+foreach my $stat_key (keys %{$malloc_stats->{'total'}}) {
+    if ($stat_key =~ /^malloc/) {
+        $got_malloc = 1;
+        last;
+    }
+}
+ok($got_malloc, 'malloc stats in malloc stats');
+
+my $got_misc = 0;
+foreach my $stat_key (@misc_stats_keys) {
+    if (exists $malloc_stats->{'total'}{$stat_key}) {
+        $got_misc = 1;
+        last;
+    }
+}
+ok(! $got_misc, 'no misc stats in malloc stats');
diff -rN -u Cache-Memcached-1.22.orig/t/all.t Cache-Memcached-1.22/t/all.t
--- Cache-Memcached-1.22.orig/t/all.t	2006-04-30 04:53:37.000000000 -0400
+++ Cache-Memcached-1.22/t/all.t	1969-12-31 19:00:00.000000000 -0500
@@ -1,38 +0,0 @@
-# -*-perl-*-
-
-use strict;
-use Test::More;
-use Cache::Memcached;
-use IO::Socket::INET;
-
-my $testaddr = "127.0.0.1:11211";
-my $msock = IO::Socket::INET->new(PeerAddr => $testaddr,
-                                  Timeout  => 3);
-if ($msock) {
-    plan tests => 9;
-} else {
-    plan skip_all => "No memcached instance running at $testaddr\n";
-    exit 0;
-}
-
-my $memd = Cache::Memcached->new({
-    servers   => [ $testaddr ],
-    namespace => "Cache::Memcached::t/$$/" . (time() % 100) . "/",
-});
-
-ok($memd->set("key1", "val1"), "set succeeded");
-
-is($memd->get("key1"), "val1", "get worked");
-ok(! $memd->add("key1", "val-replace"), "add properly failed");
-ok($memd->add("key2", "val2"), "add worked on key2");
-is($memd->get("key2"), "val2", "get worked");
-
-ok($memd->replace("key2", "val-replace"), "replace worked");
-ok(! $memd->replace("key-noexist", "bogus"), "replace failed");
-
-my $stats = $memd->stats;
-ok($stats, "got stats");
-is(ref $stats, "HASH", "is a hashref");
-
-
-
diff -rN -u Cache-Memcached-1.22.orig/t/use.t Cache-Memcached-1.22/t/use.t
--- Cache-Memcached-1.22.orig/t/use.t	2006-03-23 13:39:36.000000000 -0500
+++ Cache-Memcached-1.22/t/use.t	1969-12-31 19:00:00.000000000 -0500
@@ -1,7 +0,0 @@
-#!/usr/bin/env perl -w
-use strict;
-use Test;
-BEGIN { plan tests => 1 }
-
-use Cache::Memcached; ok(1);
-exit;


More information about the memcached mailing list