Code review comment for lp:~lifeless/launchpad/lognamer

Thanks for the review.

On Wed, Jun 30, 2010 at 7:55 AM, Curtis Hovey
<email address hidden> wrote:
> Review: Needs Information
> Hi Rob.
>
> Thanks for extracting this logic. I have a few comments and I am confused
> by a test change.

Cool :)

>> === added file 'lib/canonical/launchpad/webapp/lognamer.py'
>> --- lib/canonical/launchpad/webapp/lognamer.py        1970-01-01 00:00:00 +0000
>> +++ lib/canonical/launchpad/webapp/lognamer.py        2010-06-27 06:41:28 +0000
>
> webapp is deprecated. I think this would be better located at
> lib/lp/services/logger/lognamer.py, maybe logger is too broad.

Some similar things that might want to use this:
log / dump / debug / trace / audit

This is infrastructure for them, its about uniquely managing a set of
files on disk, in date and lexical ascending order.

So perhaps
logger/filemanager.py namemanager / namer / controller /

>> +class LogNamer:
>> +    """Assign unique file names to logs being written from an app/script.
>> +
>> +    LogNamer causes logs written from one process to be uniquely named. It is not
>> +    safe for use in multiple processes with the same output root - each process
>> +    must have a unique output root.
>> +    """
>
> I see trailing whitespace. Wrap the doc at 78 characters.

Sorry, editor isn't set to hunt out trailing whitespace, will fix and rewrap.

>> +    def getFilename(self, log_serial, time):
>> +        """Get the filename for a given log serial and time."""
>> +        log_subtype = self._log_infix()
>> +        output_dir = self.output_dir(time) #XXX locks and scans, bad.
>
> Avoid end comments. This looks like a comment rather than an XXX to denote
> tech-debt.

Ok. Gary just reminded me that you have a policy of needing a bug for
XXX, should I file one ?

>> +    def newId(self, now=None):
>> +        """Returns an (id, filename) pair for use by the caller.
>> +
>> +        The ID is composed of a short string to identify the Launchpad instance
>> +        followed by an ID that is unique for the day.
>> +
>> +        The filename is composed of the zero padded second in the day
>> +        followed by the ID.  This ensures that reports are in date order when
>> +        sorted lexically.
>> +        """
>> +        if now is not None:
>> +            now = now.astimezone(UTC)
>> +        else:
>> +            now = datetime.datetime.now(UTC)
>> +        # We look up the error directory before allocating a new ID,
>> +        # because if the day has changed, errordir() will reset the ID
>> +        # counter to zero.
>> +        self.output_dir(now)
>> +        self._lock.acquire()
>> +        try:
>> +            self._last_serial += 1
>> +            newid = self._last_serial
>> +        finally:
>> +            self._lock.release()
>> +        subtype = self._log_infix()
>> +        day_number = (now - epoch).days + 1
>> +        log_id = '%s-%d%s%d' % (self._log_type, day_number, subtype, newid)
>> +        filename = self.getFilename(newid, now)
>> +        return log_id, filename
>
> I am confused. The only changes I see in this method is prefix => infix,
> yet I can see you changed the the last test for this method to verify
> an error was raised.

> Was the test wrong? Am I reading ``now.astimezone(UTC)`` wrong?

I think you are misreading:
the old test was:
- # Another oops with a native datetime.
- now = datetime.datetime(2006, 04, 02, 00, 30, 00)
- self.assertRaises(ValueError, utility.newOopsId, now)

The new test:
+ # Native timestamps are not permitted - UTC only.
+ now = datetime.datetime(2006, 04, 02, 00, 30, 00)
+ self.assertRaises(ValueError, namer.newId, now)

I think it should be rephrased though, to say 'timestamps that cannot
be coerced into UTC are not accepted' - that would avoid confusing
people I hope.

Thanks,
Rob