small improvements #14
Conversation
3 similar comments
|
I don't think these are that great improvements. This patch deprecates one method that is guaranteed in use everywhere. Creates an memory cache that doesn't invalidate correctly and changes the function of get_valid into get_current. If you absolutely don't want to parse the lease file twice you can always use the IscDhcpLeases.get() method to get all leases and filter them in your own application. The other methods are only some shortcuts for much used usecases. Maybe @andir has some insights in this since he has some more high performance requirements. |
|
I would appreciate if we could have some kind of caching between the different filtered lists (active, valid, ..). Parsing my leases files already takes a lot of time and doing it twice isn't ideal. Currently I've re-implemented the get_current method in my code since I'm generating several metrics & actions from the raw list. So caching would be one thing I'd like to see happen. Regarding the deprecation of A simple methods retrieving all the leases would probably be sufficient. We could return a Result-DHCPLeases-Type which just holds all the parsed leases. The class could be sub-classed based on LeaseV4 or LeaseV6. On the Result-Type we could offer various "filters" like the currently provided "current". Also filters like "active" and maybe "inactive" would be of use. I could imagine the user code to look like this: leases = dhcp_leases.parse('/var/lib/dhcpd/leases.db')
type(leases) in [LeasesV4, LeasesV6]
# access properties providing lazily calculated lists of leases
leases.current
leases.active
leases.inactive
# some filters (just quick ideas so far)
leases.from_hardware('aa:bb:cc:dd:ee:ff')
leases.for('192.2.0.145')
# we could even make them chainable without much effort
l = leases.for('192.2.0.145')
l.valid
l.invalid
l.currentI've to admit that I was sitting next to @psy and was throwing some ideas onto him. I didn't really have motivation to do a lot of code-reviewing that day :-) I'm willing to work on this (with whoever may be up for the task). |
|
That looks like a neat api. If you break the api for new features then break it a lot for big improvements. The current api is design to just parse the file and get the raw data to process somewhere else. Some chainable filtering would be nice if you filter the data in the code directly. That would probably be easily implementable. Every filter returns a new LeasesV4 or LeasesV6 object which also has the side effect of caching the data. The seperate classes for v4 and v6 make it less easy to do get the ipv4 and ipv6 leases for the same mac address. Maybe just return a Leases class that can hold both v4 and v6 leases. Your example code doesn't show how the leases themselves look and how you retrieve them from the LeasesVx class. Maybe something like this: >>> #Parse one or more lease files
>>> leases = dhcp_leases.parse(['/var/lib/dhcpd/leases.db', '/var/lib/dhcpd/leases6.db'])
>>> leases
<Leases ...>
>>> # get all active v4 leases
>>> leases.v4.active
<Leases ...>
>>> list(_)
[<LeaseV4 1.2.3.4>, <LeaseV4 2.3.4.5>]
>>> # get all addresses for mac
>>> leases.for(hardware='aa:bb:cc:dd:ee:ff')
<Leases ...>
>>> list(l)
[<LeaseV4 1.2.3.4>, <LeaseV6 fe80::1>]This all will make it work more like a ORM thing for dhcp leases than a simple parser. |
|
I agree it adds a lot of complexity to the library. So why not start out by writing a new library that uses the current parsing lib and if we feel it is okay/sane/good/$metric merge it in as an addition while still keeping the old API ? |
|
I've written a simple example of what I thought would fit our ideas: https://github.com/andir/isc-dhcp-filter/blob/master/isc_dhcp_filter/__init__.py The tests show how it can be used: https://github.com/andir/isc-dhcp-filter/blob/master/tests/__init__.py |
|
In the mean-time I've made my hackup a proper library which can be installed via pip: |
4 participants
No description provided.