[yast-devel] Some notes about new storage API
Once upon a time... =================== As part of the PBI titles "Evaluate design of future-proof storage subsystem", I took a look to the content of https://github.com/aschnell/libstorage-bgl-eval/ This mail can be considered to some extend a follow up of previous evaluations and discussions in this mailing list. Most feedback received from those threads has been already taken into account into the current prototype. Thus, some topics discussed there are outdated but some information is still useful. http://lists.opensuse.org/yast-devel/2014-10/msg00045.html http://lists.opensuse.org/yast-devel/2014-12/msg00016.html First impression ================ First thing I noticed in that repository is the lack of information about building and installing. In every github's README I expect to find: (1) how to build the thing, (2) how to compile the inline docs or a link to an up-to-date compiled version, (3) in (open)SUSE related stuff I also expect instructions to download or build a rpm. That being said, I took a look to the Ruby examples and testsuite and in general I liked what I saw a lot. So first of all, congrats for the great work! Only some (maybe too Ruby-centric) comments follow. Casting and Storage.some_method =============================== Things like this look quite un-ruby: tmp1 = devicegraph.find_device(42) assert(Storage.disk?(tmp1)) assert(Storage.to_disk(tmp1)) assert(!Storage.partition_table?(tmp1)) assert_raises(Storage.DeviceHasWrongType { Storage.to_partition_table(tmp1) } I would have expected things like tmp1.disk? tmp1.to_partition_table In fact, I wouldn't expect to have to perform this kind of casts, at most I would expect to do more rubist things like tmp1.is_a?(Storage::Disk) tmp1.respond_to?(:whatever_method) Query interface =============== The API contains methods like these: device_graph.find_device(numeric_id) Storage::BlkDevice.find(device_graph, "/dev/sda") Storage::Filesystem.find_by_mountpoint(device_graph, "/") Storage::Filesystem.find_by_label(device_graph, "someLabel") Questions that come to my mind. a) Is device_graph always an object representing the whole graph? Can I do things like this to restrict the search to a subtree? disk1 = Storage::Disk.find(device_graph, "/dev/sda") sub_graph = disk1.to_device_graph # or maybe sub_graph = device_graph.sub_graph(disk1.sid) # or any other thing that makes sense Storage::Filesystem.find_by_mountpoint(sub_graph, "/") 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. :-) c) Just brainstorming. The approach in (b) could be pushed to the limit to have things like this. But actually I'm not sure if it's a good idea, just braindump. device_graph.find(sid: numeric_id) device_graph.find(type: :filesystem, mountpoint: "/") Lack of inline documentation ============================ Right now, the code is the documentation. I have to say that the code is very nicely structured, so it was really easy to me to browse for the methods I wanted to check (and they were all fairly small and understandable as well). But it's time to start adding documentation to the whole thing. Nitpicking: boolean as parameters ================================= This is something that was already mentioned by Martin in one of the mails referred at the top. Calls like this are not 100% obvious sda.descendants(false) It would be nicer to have something like sda.descendants(:direct) or sda.descendants(direct: true) Nickpicking: 42 =============== I found the usage of 42 and 43 for the sids in examples and tests a little bit confusing. I grepped the source code and found that we simply use autoincremental integer to assign the sids starting with 42. Nothing against it, but please make it more obvious (and robust, in case we decide 42 is not cool anymore) in the testsuites. :-) Epilogue ======== That's all folks! -- Ancor González Sosa YaST Team at SUSE Linux GmbH -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Wed, Oct 07, 2015 at 02:12:44PM +0200, Ancor Gonzalez Sosa wrote:
As part of the PBI titles "Evaluate design of future-proof storage subsystem", I took a look to the content of https://github.com/aschnell/libstorage-bgl-eval/
This mail can be considered to some extend a follow up of previous evaluations and discussions in this mailing list. Most feedback received from those threads has been already taken into account into the current prototype. Thus, some topics discussed there are outdated but some information is still useful. http://lists.opensuse.org/yast-devel/2014-10/msg00045.html http://lists.opensuse.org/yast-devel/2014-12/msg00016.html
First impression ================
First thing I noticed in that repository is the lack of information about building and installing. In every github's README I expect to find: (1) how to build the thing, (2) how to compile the inline docs or a link to an up-to-date compiled version, (3) in (open)SUSE related stuff I also expect instructions to download or build a rpm.
It's a prototype and development was halted abrupt eight months ago.
Only some (maybe too Ruby-centric) comments follow.
Casting and Storage.some_method ===============================
Things like this look quite un-ruby:
tmp1 = devicegraph.find_device(42) assert(Storage.disk?(tmp1)) assert(Storage.to_disk(tmp1)) assert(!Storage.partition_table?(tmp1)) assert_raises(Storage.DeviceHasWrongType { Storage.to_partition_table(tmp1) }
I would have expected things like
tmp1.disk? tmp1.to_partition_table
At least in C++ such a interface looks like a very bad idea since for *every* new class you have to modify the base class. With such a design ABI stability is not possible.
In fact, I wouldn't expect to have to perform this kind of casts,
These cast should not be needed often as I already wrote in http://lists.opensuse.org/yast-devel/2014-12/msg00025.html.
at most I would expect to do more rubist things like
tmp1.is_a?(Storage::Disk) tmp1.respond_to?(:whatever_method)
Query interface ===============
The API contains methods like these:
device_graph.find_device(numeric_id) Storage::BlkDevice.find(device_graph, "/dev/sda") Storage::Filesystem.find_by_mountpoint(device_graph, "/") Storage::Filesystem.find_by_label(device_graph, "someLabel")
Questions that come to my mind.
a) Is device_graph always an object representing the whole graph?
The devicegraph always represents the whole graph.
Can I do things like this to restrict the search to a subtree?
disk1 = Storage::Disk.find(device_graph, "/dev/sda") sub_graph = disk1.to_device_graph # or maybe sub_graph = device_graph.sub_graph(disk1.sid) # or any other thing that makes sense
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.
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.
c) Just brainstorming. The approach in (b) could be pushed to the limit to have things like this. But actually I'm not sure if it's a good idea, just braindump.
device_graph.find(sid: numeric_id) device_graph.find(type: :filesystem, mountpoint: "/")
Lack of inline documentation ============================
Right now, the code is the documentation. I have to say that the code is very nicely structured, so it was really easy to me to browse for the methods I wanted to check (and they were all fairly small and understandable as well). But it's time to start adding documentation to the whole thing.
It's a prototype and development was halted abrupt eight months ago.
Nitpicking: boolean as parameters =================================
This is something that was already mentioned by Martin in one of the mails referred at the top. Calls like this are not 100% obvious
sda.descendants(false)
It would be nicer to have something like sda.descendants(:direct) or sda.descendants(direct: true)
Looks difficult as already mentioned in http://lists.opensuse.org/yast-devel/2014-12/msg00025.html
Nickpicking: 42 ===============
I found the usage of 42 and 43 for the sids in examples and tests a little bit confusing. I grepped the source code and found that we simply use autoincremental integer to assign the sids starting with 42.
Nothing against it, but please make it more obvious (and robust, in case we decide 42 is not cool anymore) in the testsuites. :-)
There is no technical reason to change the initial value for the storage id so FMPOV spending time on changing the testsuites is not justified. Regards, Arvin -- Arvin Schnell, <aschnell@suse.com> Senior Software Engineer, Research & Development SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) Maxfeldstraße 5 90409 Nürnberg Germany -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On 10/09/2015 02:54 PM, Arvin Schnell wrote:
On Wed, Oct 07, 2015 at 02:12:44PM +0200, Ancor Gonzalez Sosa wrote:
[,,,]
Only some (maybe too Ruby-centric) comments follow.
Casting and Storage.some_method ===============================
Things like this look quite un-ruby:
tmp1 = devicegraph.find_device(42) assert(Storage.disk?(tmp1)) assert(Storage.to_disk(tmp1)) assert(!Storage.partition_table?(tmp1)) assert_raises(Storage.DeviceHasWrongType { Storage.to_partition_table(tmp1) }
I would have expected things like
tmp1.disk? tmp1.to_partition_table
At least in C++ such a interface looks like a very bad idea since for *every* new class you have to modify the base class. With such a design ABI stability is not possible.
Ok.
In fact, I wouldn't expect to have to perform this kind of casts,
These cast should not be needed often as I already wrote in http://lists.opensuse.org/yast-devel/2014-12/msg00025.html.
at most I would expect to do more rubist things like
tmp1.is_a?(Storage::Disk) tmp1.respond_to?(:whatever_method)
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.
Query interface ===============
The API contains methods like these:
device_graph.find_device(numeric_id) Storage::BlkDevice.find(device_graph, "/dev/sda") Storage::Filesystem.find_by_mountpoint(device_graph, "/") Storage::Filesystem.find_by_label(device_graph, "someLabel")
Questions that come to my mind.
a) Is device_graph always an object representing the whole graph?
The devicegraph always represents the whole graph.
Can I do things like this to restrict the search to a subtree?
disk1 = Storage::Disk.find(device_graph, "/dev/sda") sub_graph = disk1.to_device_graph # or maybe sub_graph = device_graph.sub_graph(disk1.sid) # or any other thing that makes sense
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.
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. 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.
c) Just brainstorming. The approach in (b) could be pushed to the limit to have things like this. But actually I'm not sure if it's a good idea, just braindump.
device_graph.find(sid: numeric_id) device_graph.find(type: :filesystem, mountpoint: "/")
Lack of inline documentation ============================
Right now, the code is the documentation. I have to say that the code is very nicely structured, so it was really easy to me to browse for the methods I wanted to check (and they were all fairly small and understandable as well). But it's time to start adding documentation to the whole thing.
It's a prototype and development was halted abrupt eight months ago.
Nitpicking: boolean as parameters =================================
This is something that was already mentioned by Martin in one of the mails referred at the top. Calls like this are not 100% obvious
sda.descendants(false)
It would be nicer to have something like sda.descendants(:direct) or sda.descendants(direct: true)
Looks difficult as already mentioned in http://lists.opensuse.org/yast-devel/2014-12/msg00025.html
Ok.
[...]
Cheers. -- Ancor González Sosa YaST Team at SUSE Linux GmbH -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Tue, 13 Oct 2015 08:55:48 +0200 Ancor Gonzalez Sosa <ancor@suse.de> wrote:
On 10/09/2015 02:54 PM, Arvin Schnell wrote:
On Wed, Oct 07, 2015 at 02:12:44PM +0200, Ancor Gonzalez Sosa wrote:
[,,,]
Only some (maybe too Ruby-centric) comments follow.
Casting and Storage.some_method ===============================
Things like this look quite un-ruby:
tmp1 = devicegraph.find_device(42) assert(Storage.disk?(tmp1)) assert(Storage.to_disk(tmp1)) assert(!Storage.partition_table?(tmp1)) assert_raises(Storage.DeviceHasWrongType { Storage.to_partition_table(tmp1) }
I would have expected things like
tmp1.disk? tmp1.to_partition_table
At least in C++ such a interface looks like a very bad idea since for *every* new class you have to modify the base class. With such a design ABI stability is not possible.
Ok.
In general in object desing you do not need to have such questioning. And if so, then maybe questioning like `MDRaid.can_use?(obj)` so it questioning ability and not type.
In fact, I wouldn't expect to have to perform this kind of casts,
These cast should not be needed often as I already wrote in http://lists.opensuse.org/yast-devel/2014-12/msg00025.html.
at most I would expect to do more rubist things like
tmp1.is_a?(Storage::Disk) tmp1.respond_to?(:whatever_method)
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".
Query interface ===============
The API contains methods like these:
device_graph.find_device(numeric_id) Storage::BlkDevice.find(device_graph, "/dev/sda") Storage::Filesystem.find_by_mountpoint(device_graph, "/") Storage::Filesystem.find_by_label(device_graph, "someLabel")
Questions that come to my mind.
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. What I expect is object like API that looks like device_graph.find_device(id) device_graph.find_device("/dev/sda", block: true) device_graph.mount_points.get("/") device_graph.labels.get("someLabel", filesystem: true) this way it is more separated responsibility. In general now device_graph looks like target map, which I would like to avoid. I prefer to have graph of objects that points to each other and it is easy to say what object it returns. It also allows easy introspection and better documentation that allows easier usability.
Can I do things like this to restrict the search to a subtree?
disk1 = Storage::Disk.find(device_graph, "/dev/sda") sub_graph = disk1.to_device_graph # or maybe sub_graph = device_graph.sub_graph(disk1.sid) # or any other thing that makes sense
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. 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")`
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" }
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.
c) Just brainstorming. The approach in (b) could be pushed to the limit to have things like this. But actually I'm not sure if it's a good idea, just braindump.
device_graph.find(sid: numeric_id) device_graph.find(type: :filesystem, mountpoint: "/")
Lack of inline documentation ============================
Right now, the code is the documentation. I have to say that the code is very nicely structured, so it was really easy to me to browse for the methods I wanted to check (and they were all fairly small and understandable as well). But it's time to start adding documentation to the whole thing.
I think that code is the best documentation if API is small and have good names. Josef
It's a prototype and development was halted abrupt eight months ago.
Nitpicking: boolean as parameters =================================
This is something that was already mentioned by Martin in one of the mails referred at the top. Calls like this are not 100% obvious
sda.descendants(false)
It would be nicer to have something like sda.descendants(:direct) or sda.descendants(direct: true)
Looks difficult as already mentioned in http://lists.opensuse.org/yast-devel/2014-12/msg00025.html
Ok.
[...]
Cheers.
-- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
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?
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.
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.
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.
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.
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.
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.
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.
I think that code is the best documentation if API is small and have good names.
:) ciao Arvin -- Arvin Schnell, <aschnell@suse.com> Senior Software Engineer, Research & Development SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) Maxfeldstraße 5 90409 Nürnberg Germany -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Tue, 13 Oct 2015 11:29:44 +0200 Arvin Schnell <aschnell@suse.com> 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@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Tue, Oct 13, 2015 at 01:05:16PM +0200, Josef Reidinger wrote:
On Tue, 13 Oct 2015 11:29:44 +0200 Arvin Schnell <aschnell@suse.com> 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.
Sure it's a graph, see https://en.wikipedia.org/wiki/Graph_%28mathematics%29, esp. "a graph is a [...] pair G = (V, E) comprising a set V of vertices [...] together with a set E of edges [...]".
Regarding god objest see https://en.wikipedia.org/wiki/God_object it is one extrem, where you have too powerfull object.
I consider a single search function, as was proposed, as too powerful (among other problems already explained).
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?
These general rules don't help much when we discuss concrete examples.
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
That is not a graph but just a list of nodes without edges.
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.
In a graph everything can point to everything. The "pointer" can even hold data.
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.
In the current prototype the functions can work on all graphs, whether it's the "system", the "staging" or "my ideas" graph.
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.
Sorry, I don't understand that.
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.
If I get you right you want to replace documentation by introspecion.
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.
The documentation will include a tree of storage classes so you can get that information very easy.
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("/")
Then you don't understand the basic prinicipe of OOP, that the methods are part of the class that hold the data.
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.
It should not happen so often. And you will have to add the getter and setter anyway. ciao Arvin -- Arvin Schnell, <aschnell@suse.com> Senior Software Engineer, Research & Development SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) Maxfeldstraße 5 90409 Nürnberg Germany -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Tue, 13 Oct 2015 14:54:40 +0200 Arvin Schnell <aschnell@suse.com> wrote:
On Tue, Oct 13, 2015 at 01:05:16PM +0200, Josef Reidinger wrote:
On Tue, 13 Oct 2015 11:29:44 +0200 Arvin Schnell <aschnell@suse.com> 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.
Sure it's a graph, see https://en.wikipedia.org/wiki/Graph_%28mathematics%29, esp. "a graph is a [...] pair G = (V, E) comprising a set V of vertices [...] together with a set E of edges [...]".
OK, to be precise graph of objects, not just data.
Regarding god objest see https://en.wikipedia.org/wiki/God_object it is one extrem, where you have too powerfull object.
I consider a single search function, as was proposed, as too powerful (among other problems already explained).
Question is if for user is easier to learn and understand powerful method or powerful object, that have bunch of methods.
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?
These general rules don't help much when we discuss concrete examples.
I state specific part in first paragraph. Problem with this design is when data is changed, so now any change of target_map affect too much code. Same problem is in new design with device_graph, which affect too much part of code.
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
That is not a graph but just a list of nodes without edges.
I just want to demonstrate why it is god object and flat desing. device_graph point to everything. So it is god or big brother :)
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.
In a graph everything can point to everything. The "pointer" can even hold data.
Difference is who knows about connection. If graph knows about connections or nodes.
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.
In the current prototype the functions can work on all graphs, whether it's the "system", the "staging" or "my ideas" graph.
And is it reusable code? Now all code depends heavily on device_graph. So if I want reuse some class elsewhere I need big god object like device_graph. If it is used by smaller classes I know that for reusability I need implement few abstract classes or in ruby case have objects that responds to given set of methods.
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.
Sorry, I don't understand that.
My point is that class in object design is encapsulated data and ability it provides, so for usage you check methods of class. But device_graph have a lot of methods that lives in different place.
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.
If I get you right you want to replace documentation by introspecion.
In general no. introspection is to get ability of object. Documentation is to get details of abilities. Introspection is useful when you want extendable interface.
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.
The documentation will include a tree of storage classes so you can get that information very easy.
Yes, if there is no extensions and if user know what class he want.
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("/")
Then you don't understand the basic prinicipe of OOP, that the methods are part of the class that hold the data.
And thats the point. I think it is wrong if all data is hold by device_graph. It is not clear for me how extension can extend data if it is all hold by one class. And BTW how Storage::Filesystem class holds data to find mountpoint? https://github.com/aschnell/libstorage-bgl-eval/blob/master/storage/Devices/... shows that it use data from device graph, which holds all data.
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.
It should not happen so often. And you will have to add the getter and setter anyway.
ciao Arvin
I think in general conclusion for me is that I do not like data centric api with device_graph and we do not agreed on that, so I am interested also in others opinion. Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Tue, Oct 13, 2015 at 03:19:45PM +0200, Josef Reidinger wrote:
On Tue, 13 Oct 2015 14:54:40 +0200 Arvin Schnell <aschnell@suse.com> wrote:
On Tue, Oct 13, 2015 at 01:05:16PM +0200, Josef Reidinger wrote:
On Tue, 13 Oct 2015 11:29:44 +0200 Arvin Schnell <aschnell@suse.com> 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.
Sure it's a graph, see https://en.wikipedia.org/wiki/Graph_%28mathematics%29, esp. "a graph is a [...] pair G = (V, E) comprising a set V of vertices [...] together with a set E of edges [...]".
OK, to be precise graph of objects, not just data.
But the device graph does hold objects and all the storage data is in that objects. Just look at Devicegraph.h and you can see that it is a small class.
I consider a single search function, as was proposed, as too powerful (among other problems already explained).
Question is if for user is easier to learn and understand powerful method or powerful object, that have bunch of methods.
The design has a many objects with few functions.
That is something different. Now you have one object that holds it. so it is like
device_graph -> A, B, C, D, E
That is not a graph but just a list of nodes without edges.
I just want to demonstrate why it is god object and flat desing. device_graph point to everything. So it is god or big brother :)
No, it is not. The device graph does not directly contain all storage data.
Difference is who knows about connection. If graph knows about connections or nodes.
Suppose you have a system with ten empty disks and you represent those not in a graph or list. How can you iterate over all disks?
In the current prototype the functions can work on all graphs, whether it's the "system", the "staging" or "my ideas" graph.
And is it reusable code? Now all code depends heavily on device_graph. So if I want reuse some class elsewhere I need big god object like device_graph. If it is used by smaller classes I know that for reusability I need implement few abstract classes or in ruby case have objects that responds to given set of methods.
Many functions simply cannot work without more knowledge of the graph. E.g. partition functions need the partition table type, logical volume functions need the name of the volume group, etc. So the objects have a backreference to the graph on purpose. Otherwise you would indeed end up with objects that just hold the storage data and a bunch of floating functions taking as the first two arguments the device graph and object. That sounds like a horrible interface.
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.
Sorry, I don't understand that.
My point is that class in object design is encapsulated data and ability it provides, so for usage you check methods of class. But device_graph have a lot of methods that lives in different place.
Do you look at Array when you want to know that you can do with the Person stored in Array<Person>? The graph is just a container.
If I get you right you want to replace documentation by introspecion.
In general no. introspection is to get ability of object. Documentation is to get details of abilities. Introspection is useful when you want extendable interface.
Then I don't understand you.
And thats the point. I think it is wrong if all data is hold by device_graph. It is not clear for me how extension can extend data if it is all hold by one class.
It is not all in Devicegraph. Look at the class. Look at Filesystem class. Where is the filesystem label and UUID?
And BTW how Storage::Filesystem class holds data to find mountpoint? https://github.com/aschnell/libstorage-bgl-eval/blob/master/storage/Devices/... shows that it use data from device graph, which holds all data.
No, it uses Devicegraph as a container but the mountpoint is in Filesystem.
I think in general conclusion for me is that I do not like data centric api with device_graph and we do not agreed on that, so I am interested also in others opinion.
The design is not data centric. The general conclusion for me is that this thread gives no new insight and unless really new ideas come up I will not comment anymore. ciao Arvin -- Arvin Schnell, <aschnell@suse.com> Senior Software Engineer, Research & Development SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) Maxfeldstraße 5 90409 Nürnberg Germany -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Tue, Oct 13, 2015 at 08:55:48AM +0200, Ancor Gonzalez Sosa wrote:
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.
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()
1. This API converts compile-time checks to runtime errors, e.g. if you search for a flag that does not exist. 2. It's not type-safe in that the s.first() cannot return a Filesystem object but only a Device and then you need all the manual casts you don't want. 3. The search function must be adapted for every new object and flag. Why is that be better than adding a find_by_xx function (in the class where xx is)?
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.
The set of classes is not fixed. On research already people requested to have plugins to support new storage types so any design that has a central search function like above looks inappropriate. ciao Arvin -- Arvin Schnell, <aschnell@suse.com> Senior Software Engineer, Research & Development SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) Maxfeldstraße 5 90409 Nürnberg Germany -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Tue, 13 Oct 2015 10:43:09 +0200 Arvin Schnell <aschnell@suse.com> wrote:
On Tue, Oct 13, 2015 at 08:55:48AM +0200, Ancor Gonzalez Sosa wrote:
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.
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()
1. This API converts compile-time checks to runtime errors, e.g. if you search for a flag that does not exist.
agreed. It should be what kind you search for. So it is better to have something like search_filesystem
2. It's not type-safe in that the s.first() cannot return a Filesystem object but only a Device and then you need all the manual casts you don't want
above solve it. .
3. The search function must be adapted for every new object and flag. Why is that be better than adding a find_by_xx function (in the class where xx is)?
Difference is that it should define Kind and not Target object. Basically with find_by_xxx we are back in target_map times.
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.
The set of classes is not fixed. On research already people requested to have plugins to support new storage types so any design that has a central search function like above looks inappropriate.
Basically for plugins the best design is to have good concept base class like filesystem, container that have methods that allows them to connect to rest of graph In general I think we should follow already know principles of good OO design like open/closed principle https://en.wikipedia.org/wiki/Open/closed_principle , SOLID or KISS Josef
ciao Arvin
-- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Tue, Oct 13, 2015 at 10:56:15AM +0200, Josef Reidinger wrote:
On Tue, 13 Oct 2015 10:43:09 +0200 Arvin Schnell <aschnell@suse.com> wrote:
On Tue, Oct 13, 2015 at 08:55:48AM +0200, Ancor Gonzalez Sosa wrote:
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.
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()
1. This API converts compile-time checks to runtime errors, e.g. if you search for a flag that does not exist.
agreed. It should be what kind you search for. So it is better to have something like search_filesystem
So Filesystem::find_by_mountpoint(): Type-safe and in the class Filesystem where it belongs.
The set of classes is not fixed. On research already people requested to have plugins to support new storage types so any design that has a central search function like above looks inappropriate.
Basically for plugins the best design is to have good concept base class like filesystem, container that have methods that allows them to connect to rest of graph
In general I think we should follow already know principles of good OO design like open/closed principle https://en.wikipedia.org/wiki/Open/closed_principle , SOLID or KISS
Good, so I expect that all "improvement ideas" show that they follow those principles. So far that was unfortunately not the case. ciao Arvin -- Arvin Schnell, <aschnell@suse.com> Senior Software Engineer, Research & Development SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) Maxfeldstraße 5 90409 Nürnberg Germany -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Wed, Oct 07, 2015 at 02:12:44PM +0200, Ancor Gonzalez Sosa wrote:
As part of the PBI titles "Evaluate design of future-proof storage subsystem", I took a look to the content of https://github.com/aschnell/libstorage-bgl-eval/
To not repeat discussions I have created a short document with some design decisions of libstorage-bgl-eval. Surely the decisions can be discussed but at least some reasoning is already available. https://github.com/aschnell/libstorage-bgl-eval/blob/master/doc/design-decis... ciao Arvin -- Arvin Schnell, <aschnell@suse.com> Senior Software Engineer, Research & Development SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) Maxfeldstraße 5 90409 Nürnberg Germany -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On 15.10.2015 14:05, Arvin Schnell wrote:
To not repeat discussions I have created a short document with some design decisions of libstorage-bgl-eval. Surely the decisions can be discussed but at least some reasoning is already available.
https://github.com/aschnell/libstorage-bgl-eval/blob/master/doc/design-decis...
Thanks for summarizing the discussion. I'm a bit missing something like pros & cons of different ideas and their comparison to other ones, but that's probably a good start for others to add them. Bye Lukas -- Lukas Ocilka, Systems Management (Yast) Team Leader SLE Department, SUSE Linux -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Wed, Oct 07, 2015 at 02:12:44PM +0200, Ancor Gonzalez Sosa wrote:
Lack of inline documentation ============================
Right now, the code is the documentation. I have to say that the code is very nicely structured, so it was really easy to me to browse for the methods I wanted to check (and they were all fairly small and understandable as well). But it's time to start adding documentation to the whole thing.
I wanted to have an overview of the classes in the library, so I ended up fixing this aspect: https://github.com/aschnell/libstorage-bgl-eval/pull/2 (BTW I am missing a ReadTheDocs hosting site for C++/Doxygen) But I still have questions: What is a Holder? It seems to be characterized by having a source sid and a target sid, which still does not give me a hint. Its subclasses Subdevice and User are even more opaque to me. Environment sounds like a candidate for a kitchen sink of unrelated hacks. Can we describe it with the purpose of limiting its scope? Devicegraph should be renamed to DeviceGraph. It just isn't a single word. Why is there Storage::copy_devicegraph if Devicegraph is_a noncopyable? The "Encryption" name is bothering me because encryption is a concept whereas all the other subclasses of Device are things. storage::Disk::get_transport returns a storage_legacy::Transport. A "modern" object returning a "legacy" object does not feel right, but I don't know enough to suggest an alternative. -- Martin Vidner, YaST Team http://en.opensuse.org/User:Mvidner Kuracke oddeleni v restauraci je jako fekalni oddeleni v bazenu
On Thu, Oct 15, 2015 at 05:30:15PM +0200, Martin Vidner wrote:
What is a Holder? It seems to be characterized by having a source sid and a target sid, which still does not give me a hint. Its subclasses Subdevice and User are even more opaque to me.
Technical it's the edge in the graph, so it expresses what device is linked what device. The name I have borrowed from sysfs (but it's not the same as in sysfs), e.g. on my system I have (simplified) # ll /sys/block/sda/sda2/holders/ lrwxrwxrwx 1 root root 0 Oct 15 17:17 dm-0 -> ../../../../../../../../../../virtual/block/dm-0 lrwxrwxrwx 1 root root 0 Oct 15 17:17 dm-1 -> ../../../../../../../../../../virtual/block/dm-1 In the graph there are different holders. - Subdevice: E.g. a partition is the subdevice of a Disk and a logical volume is the subdevice of a volume group. - User: E.g. a partition is used by a volume group (the case on my system), a disk is used by a RAID, a logical volume is used by a filesystem. So far the distinction is not really important so it could change.
Environment sounds like a candidate for a kitchen sink of unrelated hacks. Can we describe it with the purpose of limiting its scope?
Yes, we should define its purpose.
Devicegraph should be renamed to DeviceGraph. It just isn't a single word.
Fine for me.
Why is there Storage::copy_devicegraph if Devicegraph is_a noncopyable?
Devicegraph has a copy function. Replacing the copy function by standard copy and assignment operators should be possible.
The "Encryption" name is bothering me because encryption is a concept whereas all the other subclasses of Device are things.
Is "Crypt", "CryptDevice" or "EncryptedDevice" better for you?
storage::Disk::get_transport returns a storage_legacy::Transport. A "modern" object returning a "legacy" object does not feel right, but I don't know enough to suggest an alternative.
Some enums are used in the legacy and the new interface. Defining them twice is bad so for the time being I use the legacy namespace. Nothing more. ciao Arvin -- Arvin Schnell, <aschnell@suse.com> Senior Software Engineer, Research & Development SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) Maxfeldstraße 5 90409 Nürnberg Germany -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Thu, Oct 15, 2015 at 06:17:06PM +0200, Arvin Schnell wrote:
On Thu, Oct 15, 2015 at 05:30:15PM +0200, Martin Vidner wrote:
What is a Holder? It seems to be characterized by having a source sid and a target sid, which still does not give me a hint. Its subclasses Subdevice and User are even more opaque to me.
Technical it's the edge in the graph, so it expresses what device is linked what device. The name I have borrowed from sysfs (but it's not the same as in sysfs), e.g. on my system I have (simplified)
# ll /sys/block/sda/sda2/holders/ lrwxrwxrwx 1 root root 0 Oct 15 17:17 dm-0 -> ../../../../../../../../../../virtual/block/dm-0 lrwxrwxrwx 1 root root 0 Oct 15 17:17 dm-1 -> ../../../../../../../../../../virtual/block/dm-1
In the graph there are different holders.
- Subdevice: E.g. a partition is the subdevice of a Disk and a logical volume is the subdevice of a volume group.
- User: E.g. a partition is used by a volume group (the case on my system), a disk is used by a RAID, a logical volume is used by a filesystem.
So far the distinction is not really important so it could change.
The idea is of course to have more Holder classes that even have data. E.g. a RaidUser which has a "spare device" flag or a FilesystemUser which has a "journal/log device" flag. By keeping that data in the holders you don't need special care when manipulating the graph, e.g. removing a device or copying the graph. With extra lists in the Raid or Filesystem object such special care would be needed. ciao Arvin -- Arvin Schnell, <aschnell@suse.com> Senior Software Engineer, Research & Development SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) Maxfeldstraße 5 90409 Nürnberg Germany -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Fri, Oct 16, 2015 at 11:00:19AM +0200, Arvin Schnell wrote:
On Thu, Oct 15, 2015 at 06:17:06PM +0200, Arvin Schnell wrote:
On Thu, Oct 15, 2015 at 05:30:15PM +0200, Martin Vidner wrote:
What is a Holder? It seems to be characterized by having a source sid and a target sid, which still does not give me a hint. Its subclasses Subdevice and User are even more opaque to me.
Technical it's the edge in the graph, so it expresses what device is linked what device. The name I have borrowed from sysfs (but it's not the same as in sysfs), e.g. on my system I have (simplified)
# ll /sys/block/sda/sda2/holders/ lrwxrwxrwx 1 root root 0 Oct 15 17:17 dm-0 -> ../../../../../../../../../../virtual/block/dm-0 lrwxrwxrwx 1 root root 0 Oct 15 17:17 dm-1 -> ../../../../../../../../../../virtual/block/dm-1
Ah, interesting. As an illustration, this is what I get on 2 different machines with LANG=C ls -l `find /sys/block/*/ -type l` | egrep holders\|slaves | cut -d' ' -f9- prutrz: /sys/block/dm-0/slaves/sda3 -> ../../../../pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda/sda3 /sys/block/dm-1/holders/dm-2 -> ../../dm-2 /sys/block/dm-1/holders/dm-3 -> ../../dm-3 /sys/block/dm-1/holders/dm-4 -> ../../dm-4 /sys/block/dm-1/holders/dm-5 -> ../../dm-5 /sys/block/dm-1/holders/dm-6 -> ../../dm-6 /sys/block/dm-1/slaves/sda3 -> ../../../../pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda/sda3 /sys/block/dm-2/slaves/dm-1 -> ../../dm-1 /sys/block/dm-3/slaves/dm-1 -> ../../dm-1 /sys/block/dm-4/slaves/dm-1 -> ../../dm-1 /sys/block/dm-5/holders/dm-7 -> ../../dm-7 /sys/block/dm-5/slaves/dm-1 -> ../../dm-1 /sys/block/dm-6/slaves/dm-1 -> ../../dm-1 /sys/block/dm-7/slaves/dm-5 -> ../../dm-5 /sys/block/sda/sda3/holders/dm-0 -> ../../../../../../../../../../virtual/block/dm-0 /sys/block/sda/sda3/holders/dm-1 -> ../../../../../../../../../../virtual/block/dm-1 mrakoplas: /sys/block/dm-0/slaves/loop0 -> ../../loop0 /sys/block/dm-0/slaves/loop1 -> ../../loop1 /sys/block/loop0/holders/dm-0 -> ../../dm-0 /sys/block/loop1/holders/dm-0 -> ../../dm-0
In the graph there are different holders.
- Subdevice: E.g. a partition is the subdevice of a Disk and a logical volume is the subdevice of a volume group.
- User: E.g. a partition is used by a volume group (the case on my system), a disk is used by a RAID, a logical volume is used by a filesystem.
So far the distinction is not really important so it could change.
The idea is of course to have more Holder classes that even have data. E.g. a RaidUser which has a "spare device" flag or a FilesystemUser which has a "journal/log device" flag.
By keeping that data in the holders you don't need special care when manipulating the graph, e.g. removing a device or copying the graph.
With extra lists in the Raid or Filesystem object such special care would be needed.
Thanks for the explanation. I didn't mean to object to the existence of these classes. Association classes are fine. I'm trying to find a fitting and accurate name and description for them. So is there a 1-1 correspondence between a Holder and an edge in the DeviceGraph? Then I think a little more explanatory name would be Dependency, with a description "an edge in the DeviceGraph" The subclasses make more sense to me if they are phrased as verbs: User -> Uses Subdevice -> Contains -- Martin Vidner, YaST Team http://en.opensuse.org/User:Mvidner Kuracke oddeleni v restauraci je jako fekalni oddeleni v bazenu
On Fri, Oct 16, 2015 at 01:42:13PM +0200, Martin Vidner wrote:
On Fri, Oct 16, 2015 at 11:00:19AM +0200, Arvin Schnell wrote:
On Thu, Oct 15, 2015 at 06:17:06PM +0200, Arvin Schnell wrote:
On Thu, Oct 15, 2015 at 05:30:15PM +0200, Martin Vidner wrote:
What is a Holder? It seems to be characterized by having a source sid and a target sid, which still does not give me a hint. Its subclasses Subdevice and User are even more opaque to me.
Technical it's the edge in the graph, so it expresses what device is linked what device. The name I have borrowed from sysfs (but it's not the same as in sysfs), e.g. on my system I have (simplified)
So is there a 1-1 correspondence between a Holder and an edge in the DeviceGraph? Then I think a little more explanatory name would be Dependency, with a description "an edge in the DeviceGraph"
Yes, that is a 1:1 correspondence. The edges are holders and the vertices are devices. You might have see the graph definition: https://github.com/aschnell/libstorage-bgl-eval/blob/master/storage/Devicegr...
The subclasses make more sense to me if they are phrased as verbs: User -> Uses Subdevice -> Contains
Fine for me. Should the 'Holder' also be renamed to 'Holds'. If yes, what should the directory 'Holders' be called? ciao Arvin -- Arvin Schnell, <aschnell@suse.com> Senior Software Engineer, Research & Development SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) Maxfeldstraße 5 90409 Nürnberg Germany -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Fri, Oct 16, 2015 at 02:17:48PM +0200, Arvin Schnell wrote:
On Fri, Oct 16, 2015 at 01:42:13PM +0200, Martin Vidner wrote:
So is there a 1-1 correspondence between a Holder and an edge in the DeviceGraph? Then I think a little more explanatory name would be Dependency, with a description "an edge in the DeviceGraph"
Yes, that is a 1:1 correspondence. The edges are holders and the vertices are devices. You might have see the graph definition:
https://github.com/aschnell/libstorage-bgl-eval/blob/master/storage/Devicegr...
Thanks! I haven't got to the implementation yet.
The subclasses make more sense to me if they are phrased as verbs: User -> Uses Subdevice -> Contains
Fine for me. Should the 'Holder' also be renamed to 'Holds'. If yes, what should the directory 'Holders' be called?
How about this? Dependencies/Dependency.h Dependencies/Uses.h Dependencies/Contains.h "Holds", Holder, Holding, somehow feels too concrete for an abstract base. Maybe it's just me. -- Martin Vidner, YaST Team http://en.opensuse.org/User:Mvidner Kuracke oddeleni v restauraci je jako fekalni oddeleni v bazenu
participants (5)
-
Ancor Gonzalez Sosa -
Arvin Schnell -
Josef Reidinger -
Lukas Ocilka -
Martin Vidner