Mailinglist Archive: yast-devel (79 mails)

< Previous Next >
Re: [yast-devel] The Business Case for Y-Storage - Proposal vs. Device Graph
On 03/02/2016 05:43 PM, Stefan Hundhammer wrote:
Following up on the "Refactoring madness" thread and an IRC discussion;
I had written lately:

"this is an approach I don't like at all because it focuses on the wrong
thing: All that stuff is not about creating device graphs. We are not in
the business of making or selling device graphs. Device graphs are just
a tool for what we really intend to do. Concentrating on an irrelevant
(or, to put it less harshly, behind-the-scenes) tool is most misleading
for anybody looking at that code."

I was asked to elaborate that point further, so here we go:

What is the point of what we are doing in that new yast-storage-ng? This
is really all about making a storage proposal for our installation
workflow; this is the main purpose of it all, with more specialized
issues like the expert partitioner aside.

In most of the history of YaST2, we had the concept of providing the
user with a useful, reasonable "proposal" for each aspect of the Linux
installation. It was always intentionally called "proposal" because it
is not set in stone; the user can always change it if it doesn't fit his
needs.

So we've been doing some calculations based on hardware probing, based
on product parameters (control.xml), based on guessed values for every
aspect of the installation: For the bootloader, for the software
selection, for the desktop etc. etc., and for storage: What partitions
to remove or to resize to make room for Linux, what new partitions and
file systems to create, where to mount them.

So the term "proposal" is a well-known concept in the YaST installation
for YaST developers as well as for other people in SUSE R&D, including
product and project managers, and even for some advanced users.

I think we all agree with the whole content up to this point.

So, when looking through the installation code, I would expect that to
be somehow reflected in the code; I'd expect a class "StorageProposal"
somewhere, preferably with a main method 'propose()'. And it used to be
like that in my first version of that new storage proposal.

Before the refactoring, there was a class
Yast::Storage::StorageProposal with a method called "propose"

Now, after the refactoring, there is a class
Yast::Storage::Proposal [1] also with a method called "propose"

Not such a big change and certainly not a fundamental change in the
whole approach. But I don't care that much about naming, if you want to
bring the original name (redundant with the namespace IMO) back.

It's true that in the original version of the pull request, the
responsibility of calculating the proposal was directly in
Yast::Storage::ProposalDemoClient#run
I did it because I considered that a minor problem for the time being.
The pull request was more focused in the stuff already mentioned in my
previous mails, like removing the global catalog, avoiding I/O
arguments, avoiding to have Storage.instance.probed all over the code
and so on.

Thanks god we have code reviews. You pointed out that putting that
responsibility directly on the client was an error and thus we created
the mentioned Yast::Storage::Proposal#propose and turned
ProposalDemoClient into a more dumb class relying on it. Once again, not
a change in the whole approach, just a minor responsibility relocation
and more explicit method naming. The normal outcome of a code review.

But not for long: It was changed very soon to a class
"DevicegraphGenerator". There is now also a class "RefinedDevicegraph".
One thing that is no longer present, though, is anything with "Proposal"
in the name.

DevicegraphGenerator is not a substitute for StorageProposal and never
was. The already mentioned Storage::Proposal is that substitute. In the
very first version of the pull request (the one that was corrected),
that substitute was Storage::ProposalDemoClient.

DevicegraphGenerator is a class that implements one of the 2 big steps
currently included in the generation of the proposal. First we calculate
the requirements (done by VolumesGenerator) and then we calculate a
devicegraph to fulfill those requirements (done by DevicegraphGenerator).

On the other hand, yes, there is now a refinement called
Yast::Storage::Proposal::RefinedDevicegraph [2] (
That module contains several methods to query the status of a given
devicegraph. Those methods were previously spread over classes like
DiskAnalyzer and SpaceMaker.

To me, this is not only not self-explanatory (as good code should be),
it is outright confusing. Anybody searching for the place where a
storage proposal is made will have to do a lot of grepping to find where
it happens, and so far every was vers much surprised (to put it mildly)
where this was.

Why is it more self-explanatory to find those proposal-related methods
in SpaceMaker or in DiskAnalyzer (two files located in /lib/storage, if
we talk about grep) than in Proposal::RefinedDevicegraph (a file located
in /lib/storage/proposal)?

Or do you mean that Storage::Proposal#propose is outright confusing
compared to Storage::StorageProposal#propose?

Sure Storage::ProposalDemoClient#run is less obvious (if you insist in
discussing a code that was corrected before the merge), but far from
being a fundamental problem in the whole approach. Just a problem
corrected with better names.

That's misleading IMHO. Device graphs are a useful tool in that context,
but just that: A tool. They are not the central part around everything
revolves, at least not from the perspective of a developer using that part.

I get the main idea of not exposing devicegraph. But I don't get in
which way the pre-refactoring code was better in that regard.

Yes, we now have a Proposal:RefinedDevicegraph module that other classes
has to know about. But before the refactoring those classes already had
a dependency on Devicegraph (they manipulate devicegraphs after all) and
IN ADDITION they had dependencies on SpaceMaker and DiskAnalyzer.

There is not a single class in your examples (and in the whole code, I
would say) that knows now about devicegraphs and didn't know about them
before the refactoring.

That's why I had written that we are not in the business of making or
selling device graphs, we are in the business of making useful
proposals. I miss the whole concept of proposals here. There is only one
function in the DevicegraphGenerator called proposal(), and even that
was only named like this after several people had agreed that run() is
not a good name, in particular when you have a run() function in several
classes (good luck finding the right one with 'grep'!).

Once again. You are focusing the whole discussion on a set of bad names
that were present in the original version of a pull request claiming
that it shows fundamental differences in the approach. I offered ZERO
resistance to change those names and relocate those methods.

That very first version of the pull request touched around 1,000 lines
of code in 20 different files. It did so to fight against all the
problems described in my mails (singleton abuse, global catalog, I/O
arguments, single responsibility...). Those are fundamental differences
in the approach, from my POV.

So, what we are doing now is to put very much emphasis on an
implementation detail while omitting the well-known concept.

Yes, apart from the already mentioned relevant changes. The original
version of the pull request also contained some bad names that were
corrected during code review (add JUST another 1 file and another 30
lines of code to the above-mentioned huge numbers). Focusing the
discussion on that is looking at the finger rather than the at the moon.

Claiming that those bad names reveal fundamental differences is making a
mountain out of a molehill... and what is worse, ignoring the real
mountain whatsoever.

Kind regards

Not so fast! (sorry, I always have to win the verbosity contest)... ;-)

The statement I asked you to elaborate (you know, "this is an approach I
don't like at all because it focuses on the wrong thing...") was not
written as a reply to the refactoring pull request we have ended up
discussing in this mail. It was written as a reply to my claim that this
solution (proposed by me)

factory = Yast::Storage::DevicegraphFactory.new
devicegraph = factory.create_from_file(input_file)
devicegraph.serialize(output_file)

was better than this one (present in the current code)

fake_probing = Yast::Storage::FakeProbing.new
devicegraph = fake_probing.devicegraph
Yast::Storage::FakeDeviceFactry.load_yml_file(devicegraph,input_file)
Yast::Storage::YamlWriter.write(devicegraph, output_file)

Sorry to ask again (and we should probably move the discussion out of
the mailing list now) but I still don't see how the second option is
better at hiding the devicegraphs as an implementation detail to the
outside world. In the same way I still don't see why the pre-refactoring
code was better than the refactored one in that regard.

But as said, maybe it's time to continue the discussion "live".

Cheers

[1]
https://github.com/yast/yast-storage-ng/blob/master/src/lib/storage/proposal.rb
[2]
https://github.com/yast/yast-storage-ng/blob/master/src/lib/storage/proposal/refined_devicegraph.rb

--
Ancor González Sosa
YaST Team at SUSE Linux GmbH
--
To unsubscribe, e-mail: yast-devel+unsubscribe@xxxxxxxxxxxx
To contact the owner, e-mail: yast-devel+owner@xxxxxxxxxxxx

< Previous Next >
Follow Ups
References