Re: [Maria-developers] 0d05c9f8699: Add mytop to Client component for rpm package
Hi, Anel! About the diff to 1.9, I tend to agree that our mytop is a superset. I did have some comments when looking at the diff, couldn't help it :) Here they are:
diff --git a/scripts/mytop.sh b/../mytop-1.9.1-4 index 321b79537fc..056b7a9a806 100644 --- b/../mytop-1.9.1-4 +++ a/scripts/mytop.sh @@ -1,13 +1,13 @@ -#!/usr/bin/perl -w +#!/usr/bin/env perl
I thought we're getting rid of "env perl" ?
# -# $Id: mytop,v 1.91 2012/01/18 16:49:12 mgrennan Exp $ +# $Id: mytop,v 1.99-maria6 2019/10/22 14:53:51 jweisbuch Exp $
=pod
=head1 NAME
-mytop - display MySQL server performance info like `top' +mytop - display MariaDB server performance info like `top'
may be "MariaDB/MySQL" here and below?
=cut
@@ -242,11 +253,15 @@ my $dsn;
## Socket takes precedence.
-$dsn ="DBI:mysql:database=$config{db};mysql_read_default_group=mytop;"; +if (eval {DBI->install_driver("MariaDB")}) { + $dsn = "DBI:MariaDB:database=$config{db};mariadb_read_default_group=mytop;"; +} else { + $dsn = "DBI:mysql:database=$config{db};mysql_read_default_group=mytop;"; +}
if ($config{socket} and -S $config{socket}) { - $dsn .= "mysql_socket=$config{socket}"; + $dsn .= "mariadb_socket=$config{socket}";
if you support both drivers, you need to use the correct option here
} else { @@ -1027,10 +918,15 @@ sub GetData() } }
- open L, "</proc/loadavg"; - my $l = <L>; - close L; - chomp $l; + my $l; + if (-e "/proc/loadavg") + { + ## To avoid warnings if the OS is not Linux + open (my $fh, "<", "/proc/loadavg"); + ## Only the first 3 values are interresting
rr?
+ $l = join(" ", (split /\s+/, <$fh>)[0..2]); + close $fh; + }
$last_time = $now_time;
@@ -1960,11 +1694,15 @@ sub PrintHelp() S - change slow query hightlighting t - switch to thread view (default) u - show only a specific user + V - show variables : - enter a command (not yet implemented) ! - Skip an error that has stopped replications (at your own risk) + L - show full queries (do not strip to terminal width) + w - adjust the User and DB columns width + a - toggle the progress column
-${GREEN}http://jeremy.zawodny.com/mysql/mytop/${RESET} -${GREEN}http://www.mysqlfanboy.com/mytop-3/${RESET} +Base version from ${GREEN}http://www.mysqlfanboy.com/mytop-3${RESET}
if it's mytop-3, perhaps it should have the version 3.x and not 1.99-maria6 ?
+This version comes as part of the ${GREEN}MariaDB${RESET} distribution. ];
print $help;
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Serg, Added new commit addressing your review, thanks: https://github.com/MariaDB/server/commit/660374b542639642e825079fd065333f753... On Thu, Oct 8, 2020 at 3:24 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Anel!
About the diff to 1.9, I tend to agree that our mytop is a superset. I did have some comments when looking at the diff, couldn't help it :) Here they are:
diff --git a/scripts/mytop.sh b/../mytop-1.9.1-4 index 321b79537fc..056b7a9a806 100644 --- b/../mytop-1.9.1-4 +++ a/scripts/mytop.sh @@ -1,13 +1,13 @@ -#!/usr/bin/perl -w +#!/usr/bin/env perl
I thought we're getting rid of "env perl" ?
Yap, added in patch
# -# $Id: mytop,v 1.91 2012/01/18 16:49:12 mgrennan Exp $ +# $Id: mytop,v 1.99-maria6 2019/10/22 14:53:51 jweisbuch Exp $
=pod
=head1 NAME
-mytop - display MySQL server performance info like `top' +mytop - display MariaDB server performance info like `top'
may be "MariaDB/MySQL" here and below?
Added.
=cut
@@ -242,11 +253,15 @@ my $dsn;
## Socket takes precedence.
-$dsn ="DBI:mysql:database=$config{db};mysql_read_default_group=mytop;"; +if (eval {DBI->install_driver("MariaDB")}) { + $dsn =
"DBI:MariaDB:database=$config{db};mariadb_read_default_group=mytop;";
+} else { + $dsn = "DBI:mysql:database=$config{db};mysql_read_default_group=mytop;"; +}
if ($config{socket} and -S $config{socket}) { - $dsn .= "mysql_socket=$config{socket}"; + $dsn .= "mariadb_socket=$config{socket}";
if you support both drivers, you need to use the correct option here
Added.
} else { @@ -1027,10 +918,15 @@ sub GetData() } }
- open L, "</proc/loadavg"; - my $l = <L>; - close L; - chomp $l; + my $l; + if (-e "/proc/loadavg") + { + ## To avoid warnings if the OS is not Linux + open (my $fh, "<", "/proc/loadavg"); + ## Only the first 3 values are interresting
rr?
+ $l = join(" ", (split /\s+/, <$fh>)[0..2]); + close $fh; + }
$last_time = $now_time;
@@ -1960,11 +1694,15 @@ sub PrintHelp() S - change slow query hightlighting t - switch to thread view (default) u - show only a specific user + V - show variables : - enter a command (not yet implemented) ! - Skip an error that has stopped replications (at your own risk) + L - show full queries (do not strip to terminal width) + w - adjust the User and DB columns width + a - toggle the progress column
-${GREEN}http://jeremy.zawodny.com/mysql/mytop/${RESET} -${GREEN}http://www.mysqlfanboy.com/mytop-3/${RESET} +Base version from ${GREEN}http://www.mysqlfanboy.com/mytop-3${RESET}
if it's mytop-3, perhaps it should have the version 3.x and not 1.99-maria6 ?
The mytop-3 is just a url. The same url has 1.9.1 version as the latest one. 1.99 is from PR #215 that the author contributed (9 iterations/commits in some time frame) and it is ok from my point of view.
+This version comes as part of the ${GREEN}MariaDB${RESET} distribution.
];
print $help;
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
I think the version number could be bumped to clearly not that there are major changes and that it's not a MariaDB specific version. On the commit, you have used tabs instead of spaces on the rest of the code). On 10/13/20 2:39 PM, Anel Husakovic wrote:
Hi Serg, Added new commit addressing your review, thanks: https://github.com/MariaDB/server/commit/660374b542639642e825079fd065333f753...
On Thu, Oct 8, 2020 at 3:24 PM Sergei Golubchik <serg@mariadb.org <mailto:serg@mariadb.org>> wrote:
Hi, Anel!
About the diff to 1.9, I tend to agree that our mytop is a superset. I did have some comments when looking at the diff, couldn't help it :) Here they are:
> diff --git a/scripts/mytop.sh b/../mytop-1.9.1-4 > index 321b79537fc..056b7a9a806 100644 > --- b/../mytop-1.9.1-4 > +++ a/scripts/mytop.sh > @@ -1,13 +1,13 @@ > -#!/usr/bin/perl -w > +#!/usr/bin/env perl
I thought we're getting rid of "env perl" ?
Yap, added in patch
> # > -# $Id: mytop,v 1.91 2012/01/18 16:49:12 mgrennan Exp $ > +# $Id: mytop,v 1.99-maria6 2019/10/22 14:53:51 jweisbuch Exp $ > > =pod > > =head1 NAME > > -mytop - display MySQL server performance info like `top' > +mytop - display MariaDB server performance info like `top'
may be "MariaDB/MySQL" here and below?
Added.
> > =cut > > @@ -242,11 +253,15 @@ my $dsn; > > ## Socket takes precedence. > > -$dsn ="DBI:mysql:database=$config{db};mysql_read_default_group=mytop;"; > +if (eval {DBI->install_driver("MariaDB")}) { > + $dsn = "DBI:MariaDB:database=$config{db};mariadb_read_default_group=mytop;"; > +} else { > + $dsn = "DBI:mysql:database=$config{db};mysql_read_default_group=mytop;"; > +} > > if ($config{socket} and -S $config{socket}) > { > - $dsn .= "mysql_socket=$config{socket}"; > + $dsn .= "mariadb_socket=$config{socket}";
if you support both drivers, you need to use the correct option here
Added.
> } > else > { > @@ -1027,10 +918,15 @@ sub GetData() > } > } > > - open L, "</proc/loadavg"; > - my $l = <L>; > - close L; > - chomp $l; > + my $l; > + if (-e "/proc/loadavg") > + { > + ## To avoid warnings if the OS is not Linux > + open (my $fh, "<", "/proc/loadavg"); > + ## Only the first 3 values are interresting
rr?
> + $l = join(" ", (split /\s+/, <$fh>)[0..2]); > + close $fh; > + } > > $last_time = $now_time; > > @@ -1960,11 +1694,15 @@ sub PrintHelp() > S - change slow query hightlighting > t - switch to thread view (default) > u - show only a specific user > + V - show variables > : - enter a command (not yet implemented) > ! - Skip an error that has stopped replications (at your own risk) > + L - show full queries (do not strip to terminal width) > + w - adjust the User and DB columns width > + a - toggle the progress column > > -${GREEN}http://jeremy.zawodny.com/mysql/mytop/${RESET} <http://jeremy.zawodny.com/mysql/mytop/$%7BRESET%7D> > -${GREEN}http://www.mysqlfanboy.com/mytop-3/${RESET} <http://www.mysqlfanboy.com/mytop-3/$%7BRESET%7D> > +Base version from ${GREEN}http://www.mysqlfanboy.com/mytop-3${RESET} <http://www.mysqlfanboy.com/mytop-3$%7BRESET%7D>
if it's mytop-3, perhaps it should have the version 3.x and not 1.99-maria6 ?
The mytop-3 is just a url. The same url has 1.9.1 version as the latest one. 1.99 is from PR #215 that the author contributed (9 iterations/commits in some time frame) and it is ok from my point of view.
> +This version comes as part of the ${GREEN}MariaDB${RESET} distribution. > ]; > > print $help;
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org <mailto:security@mariadb.org>
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net <mailto:maria-developers@lists.launchpad.net> Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
participants (3)
-
Anel Husakovic -
Jean Weisbuch -
Sergei Golubchik