Mailinglist Archive: yast-devel (211 mails)

< Previous Next >
Re: [yast-devel] Some notes about new storage API
On Tue, 13 Oct 2015 11:29:44 +0200
Arvin Schnell <aschnell@xxxxxxxx> wrote:

On Tue, Oct 13, 2015 at 10:14:03AM +0200, Josef Reidinger wrote:

I'll do more experiments with this as soon as the prototype is
easily installable. But looks pretty clear that it would be
fairly easy to fix the un-rubism by adding the corresponding
methods in the Ruby side in case we decide we are so purist that
we cannot live with the current API. :-) So nothing to fix in the
libstorage (C++) side.

I agree, that it make sense to have some helpers to have ruby
bindings more "ruby".

Just remember that those need documentation and test cases.

a) Is device_graph always an object representing the whole
graph?

The devicegraph always represents the whole graph.


Well, in general I do not like this API before and I also do not
like it now :)
It is not object oriented and whats more device_graph is god like
object.

Please elaborate this. What do you mean by God like object? A
object that holds all storage objects?

Yes, if you have object that hold everything it is not graph, but flat
structure. Regarding god objest see
https://en.wikipedia.org/wiki/God_object it is one extrem, where you
have too powerfull object.


What I expect is object like API that looks like

device_graph.find_device(id)

This exists.

device_graph.find_device("/dev/sda", block: true)

Not type safe.

OK, fair enough.


this way it is more separated responsibility. In general now
device_graph looks like target map, which I would like to
avoid.

Please explain that, esp. what problems you see with the existing
target map and how you still see this problems with the new
design.

Problem of target map is that it is not much flexible. It is not
object, it is more like data. And if data changed, then all methods
that use it need to adapt. In general rule for design is

1) if data is fixed and just way how it is interpreted changed, then
create data object and have methods that work on top of it.

2) if data is changed, but way how it is worked with them do not
change, then use object that represent such data.

Reason is easy changes. If you need to add new data type, is it easier
to change device_graph and all methods that do finding or use some kind
of child to represent it and connect it to rest of system?


I prefer to have graph of objects that points to each other

The device-graph is just that and the functions to query the
"pointers" exist.

That is something different. Now you have one object that holds it. so
it is like

device_graph -> A, B, C, D, E

if it use graph of object then it can look like

device_graph -> filesystems -> A, B
-> physical_devices -> C, D
-> containers -> E
and what is more important E point to C, D....C can point to partition
C1, C2 and C1 can point to A and C2 can point to B.
This way if something need disk, it can query it for partitions and
inspect its filesystems without knowing if it is teoretic device graph,
real one or modified one.
I hope I explain it clearly enough. This way device_graph should know
only about top level concepts and do not need to know about everything.


and it is easy to say what object it returns. It also allows
easy introspection and better documentation that allows easier
usability.

In general your remarks are to buzz-work like to comment on them.

OK, let me explain it better. and use ruby as example.

If I am interested what e.g. device_graph provide me, I can do
device_graph.methods which in case passing device_graph everywhere do
not should interesting stuff. Also methods is not documented in it and
lives elsewhere.

In case when you use object like approach then you can do something like

disk.methods -> partitions, label, ...

and then you can check what provide you partitions, what provide you
label, etc.

So it is easier to understand whole picture as it is layer knowledge.
You have reasonable small Disk object and if it return in some method
e.f. Partition object, then again you can check it and see what it
provides if you need it.

Counter example now is Storage module in yast2-storage which is
overloaded by methods. Current approach in new libstorage for me looks
like we have all methods for device_graph which is just enclosed in
namespaces like
Storage::Filesystem.find_by_mountpoint(device_graph, "/")

which for me is just something like
device_graph.find_filesystem_by_mountpoint("/")

just passing data object around.

What I would like to see is something like
device_graph.filesystems.find_by_mountpoint("/")
or in similar way
device_graph.mountpoints.get("/").filesystem

It looks quite similar, but there is huge difference. You have object
for list of Filesystems, list of mountpoints with own methods, which is
reusable and where single object have significantly smaller API, which
is easier to get.

I have now I explain all my buzz word I use :)


In theory there are many ways to get a subgraph, look at the
functions in Device.h.

Storage::Filesystem.find_by_mountpoint(sub_graph, "/")

Do you have a real use-case for that? Adding subgraphs looks
like a lot of work so might not be worthwhile.

No concrete example, just wondering. But the idea of restricting
the search to a subtree somehow came naturally to my mind while
reading. Like "look for a filesystem with this property, but only
in this volume group".

Probably not worth the effort, I was just curious.

For bootloader it make sense to e.g. restrict search for boot flag
for one disk, as it is disk specific flag.

Then I will look at the subgraph and filtered_graph classes of
boost.

Same way it can be interesting
to e.g. find intersections for known problem cases like
`device_graph.find(fs:
"nfs).include?(device_graph.mount_points.get("/boot")`

That's a bad example FMPOV, instead just search for /boot and
check whether it is NFS. No need to construct two filtered graphs
and check if they intersect.

OK, fair enough.



b) Adding find_by_xx at demand over time doesn't look very
future-proof. I would prefer to see something like Rails's
#find_by, i.e.

Storage::Filesystem.find(device_graph, mountpoint: "/")
Storage::Filesystem.find(device_graph, mountpoint: "/",
label: "blah")

Too ruby-centric and not easy to implement in a cross-language
way, I guess. But asking does not hurt. :-)

As I already wrote in
http://lists.opensuse.org/yast-devel/2014-12/msg00028.html if
someone takes care of bindings for a target language more things
are possible.

Yes. And it should be fairly easy to implement. But I still wonder
about how flexible and future-proof is to keep adding find_by_xx
on demand to the API.

I agree, almost all languages have maps that can be used to pass
such kind of options. In C it is usually done by flags. In C++ it
is usually done by functor. So maybe another option is to have
generic find which can take functor or lambda in way like

device_graph.mountpoints.find { |mp| mp.path == "/" || mp.label ==
"blah" }

A filtered graph constructed by a functor should be possible.

Yeap, I hope functors adds enough flexibility for extensions.


More flexible solutions would also be more complex to use for
sure. I just wonder if they are worth exploring. One obvious
solution (that would need refinement, of course) if having a
search object that accepts any number of key-pair filters. The
most primitive form just to expose the idea would be something
like (in pseudocode):

s = Storage.search(device_graph, "filesystem")
s.add_filter("mountpoint", "/")
s.first()

Or any other idea that can be implemented with a fixed set of
classes and methods instead of extending the existing classes
with new custom methods over time.

Agreed. number for methods for given object should be kept quite
low, otherwise it is hard to remember it and also learn it.

In general agreed, but if you have a get_xx and a set_xx function
a find_by_xx seems natural.

That depends how often it will be extended. Also question is if we want
only single finder or rails like finders that can be chained like

find_by_fs(:nfs).find_by_size(:greater_then =>
50MB).find_by_mountpoint(:exist)

which should find all nfs partitions, that is at least 50MB big and is
mounted to system.


I think that code is the best documentation if API is small and have
good names.

:)

ciao Arvin


Josef
--
To unsubscribe, e-mail: yast-devel+unsubscribe@xxxxxxxxxxxx
To contact the owner, e-mail: yast-devel+owner@xxxxxxxxxxxx

< Previous Next >
Follow Ups