277571 - checksetup should not always output a message about checking for flags

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Max Kanat-Alexander]

checksetup always outputs a message that it's checking for bad flag names. It
should only do that if it actually has to update the flags.

Comment 1

[Max Kanat-Alexander]

Attachment: Move the flags message to the inside of the if block

OK, here's a very simple fix that only outputs a message if we actually *do*
something.

Comment 2

[Vlad Dascalu]

Comment on attachment 170678 [details] [diff] [review]
Move the flags message to the inside of the if block

-	 print "... done.\n";
+	 print "... done.\n" unless $silent;

That's wrong. That's the kind of text that should appear to the admin if
checksetup.pl is a cron job. That block of code executes when the need to
update flags exists. The admin would want to know the result of the operation
(if the operation was "done" or not) no matter if the script is ran as silent
or not.

Comment 3

[Max Kanat-Alexander]

Attachment: Version 2

OK, vladd and I talked in IRC.

The conclusion is that $silent should be assumed to be used by people who are
running a cron job, so they want to see output if checksetup changes anything.

Comment 4

[Vlad Dascalu]

Comment on attachment 170688 [details] [diff] [review]
Version 2

It's a pitty to remove the strings from CVS since they would really be suitable
if we had a "debug" mode.

However, for now, this seems like the right thing.

Comment 5

[Dave Miller [:justdave]]

How about this:

We have a recent change to fix the emailprefs for users that don't have them. 
The initial test this makes to find out if it has to do anything is very quick
and painless.  It also was introduced after the restrictions on flag names were
changed, so we can guarantee that any installation running the emailprefs fixup
code also needs to flags fixup code (except for those who updated from CVS
between the two patches, but it won't hurt anything for them to run it one more
time, since currently we're doing it every time anyway).

So how about if we stick the entire test for flag correctness (since it seems to
take longer than emailprefs to test whether or not it has to do anything) inside
the block that does all the emailprefs corrections?  Then leave the text about
"I'm testing for bad flag names" in the output.  It gets done once, and they
never see it again.

We've pulled this trick previously on these kinds of data manipulation tests,
where something went into checksetup.pl unconditionally, then the next time we
had a schema change, we tied it to it so that it wouldn't keep running every time.

Comment 6

[Dave Miller [:justdave]]

OK, discussed this with Max on IRC, and we discovered the reason the flagtype
check is taking so long is it pulls the entire flag list out of the database and
iterates over it in Perl to check for invalid characters.  It should be possible
to test for the invalid flagtypes directly in the initial SQL statement,
reducing the amount of data going back and forth from the database.  Max is
filing another bug for that part, and this patch is good as-is, since making it
speedier will get rid of the need to only run it once.  There's about 90 or so
flagtypes defined on b.m.o, and it took a noticible amount of time to run that
part, which is why I was complaining about it running every time.  It does
confuse me *why* it took a noticible amount of time, since it shouldn't in
theory take that long to do a simple iteration through 90 items with a regexp in
perl, but it did.  Both queries (pulling everything, and only pulling the broken
ones) appear to take the same amount of time from a profiling perspective on the
database end, but the query to pull only the broken ones returns faster when
there aren't any (because there's no data to transfer).

Comment 7

[Marc Schumann [:Wurblzap]]

(In reply to comment #6)
> It should be possible to test for the invalid flagtypes directly in the initial
> SQL statement, reducing the amount of data going back and forth from the
> database.  Max is filing another bug for that part, and this patch is good

For the record: this is bug 277723.

Comment 8

[Nick Barnes]

(In reply to comment #5)
> So how about if we stick the entire test for flag correctness (since it seems to
> take longer than emailprefs to test whether or not it has to do anything)

I'm surprised that it takes very long at all.  How long does it take?  What's
the database like?  In my test database with ten thousand flag types, it takes
less than one second (on my antique desktop machine).  See the discussion on bug
277723.  It's possible that I'm not testing it properly.  Is it possible that
you're mistaken about what's taking so long?

I'm quite happy to add a SQL test checking whether there are any bad flagtype
names (although without an index on flagtypes.name it's not clear whether this
would be much faster).  But see bug 277723 for reasons to be careful with this.

Comment 9

[Shane H. W. Travis]

2.18
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.289.2.21; previous revision: 1.289.2.20
done

Tip:
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.324; previous revision: 1.323
done