[systemd-devel] Python journal reader

[Zbigniew Jędrzejewski-Szmek]

On Sun, Apr 14, 2013 at 03:28:31PM +0100, Steven Hiscocks wrote:
> On 14/04/13 03:36, David Strauss wrote:
> >I keep writing lengthy emails about how we can use this as an
> >opportunity to reduce redundancy and improve consistency, but I should
> >probably ping you on #systemd IRC to hash it out. I can't think of
> >anything elegant that doesn't involve altering the existing journal.py
> >or _reader.c code.
Hi Steven, hi David,

I read your discussion on IRC... I agree that backwards compatibility
is not (yet) something that we need to keep very. I think that the
number of people using those Python interfaces is quite small so far,
and having a nice interface is more important than some small breakage.

> >As a starter, I want to enumerate the places where
> >__REALTIME_TIMESTAMP has special handling:
> >  (1) Existing code: A C-based member function in _reader.c.
> >  (2) Existing code: A native Python conversion function in journal.py.
> >  (3) Existing code: An entry in DEFAULT_CONVERTERS.
> >  (4) Added in your patch: A key setting in get_next().
> >  (5) Added in your patch: A Python-based member function in journal.py
> >that overrides (1).
> >  (6) Added in your patch: A condition in get() that invokes (1) when
> >the specific field is requested.
> >
> >While I want to fix this bug, I also don't want six scattered special
> >cases for __REALTIME_TIMESTAMP and similar fields.
> >
> 
> Thanks for the chat earlier on IRC David, especially given your
> localtime :-).
> 
> I thought about the points discussed: keeping things similar to
> journalctl -o json output (i.e. include special fields); get,
> get_next, and get_realtime, etc methods create duplication and
> multiple ways to get the same/some values (could cause confusion);
> issue with `next` clashing with python2 __iter__ next; trying to
> stabilise the interface (note: not declared stable yet); performance
> issues with unnecessary dictionary field conversions.
> 
> I've therefore created another patch for consideration:
> https://gist.github.com/kwirk/5382783
This looks pretty nice, and I think it can be committed. Could
you create a patch with a commit message, including some of the
rationale, so that people understand what's happenning? ;)

> I've removed the get_next and get_previous methods from _Reader, as
> these didn't align with the C API. This is replaced with:`_get_all`
> private method (effectively a wrapper around C API macro
> sd_journal_{enumerate,restart}_data/SD_JOURNAL_FOREACH_DATA);
> renamed `next` method to `_next`, such to avoid name clash (and I
> believe should be private); `_previous` method for consistency.
> 
> The `get_next` and `get_previous` methods are now in Reader only.
> These call `_next` method, followed by `_get_all` and then fetch the
> special fields, before passing whole dictionary to converters. This
> makes all the traversal, and getting standard and special fields
> from the journal transparent to the user.
> 
> With the changes of removing `get_next` in _Reader, the iter methods
> don't function as you'd expect. To that end, I've moved them to
> Reader with get_next as iter next call as before.
> 
> I understand the points about performance, but don't think the
> performance hit is that bad, or critical in most use cases. These
> changes should allow a custom class to inherit _Reader, and the
> create a more optimised version if that is required. (One option if
> this is a major issue, is a custom dictionary class could be
> returned by `get_next`, which uses the __get__ method to lazy
> convert the fields on access. This is starting to get complicated
> thought...)
I think we can always do that later on. Maybe it makes sense
to document that get_next() returns something dictionary-like
(collections.abc.Mapping) that right now is just a dict, but
could become something different later on.

Zbyszek