On Fri, Nov 30, 2018 at 02:56:01PM +0100, Sebastian Wagner wrote:
Am 30.11.18 um 14:34 schrieb Sage Weil:
On Fri, 30 Nov 2018, Sebastian Wagner wrote:
All,
looks like the architecture of the mgr supporting independent modules
was quite successful :-).
In recent times, I'm seeing an increasing amount of coped copied from
other mgr modules. This has the obvious downside that a fix in one
module leaves other modules unfixed. Like for example PR #25337.
Running `pylint --disable=all --enable=duplicate-code *` within
pybind/mgr reveals, in my opinion, too many duplicates. One also have to
count the places where code was semantically copied (or inspired by).
One obvious reason is that the mgr modules are prone to replicating
code, as each module is build by many distinct teams for a specific and
independent use case. Also, testing other affected modules can be quite
challenging.
Therefor, I'd like to raise awareness. Maybe, we can reduce copies in
the future.
My main question is what our strategy should be. Where should shared code
live? Should we transplant it all into mgr_module.py, or into util.py or
similar in the pybind/mgr directory?
pybind/mgr/util.py sounds good. To me, mgr_module.py sounds like a
Python module meant to contain the API to C++.
Current example: I'm using format_bytes() from the status module from the
pg_autoscaler module via remote(), which is probably not ideal!
Perfect example! We have a very similar function in
/src/ceph-volume/ceph_volume/util/disk.py called human_readable_size. We
don't have any common place for shared Python code right now.
This is ceph-volume though. Are we talking about shared code between all python
pieces within ceph? I'm sure there're plenty of opportunities for shared
implementations within ceph, but this would be a major reorganisation and will
likely require implementation changes.
A util.py for mgr modules seems like a step in the right direction though.
s
--
Jan Fajerski
Engineer Enterprise Storage
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton,
HRB 21284 (AG Nürnberg)
