Re: bzr commit into mysql-6.0-backup branch (cbell:2638) Bug#33364, Bug#35230, WL#4296

[Rafal Somla]

Hi Chuck,

If I understand you correctly, you are basically proposing to postpone 
discussing any design related issues now and push the code as it is, because "we 
are supposed to push it in June".

But wait a moment - why are we so eager to push it now? The task description is 
"Refine backup progress mechanism". It is not fixing any serious bug. Compared 
to the current behaviour it adds a possibility of logging to the files, not only 
to tables. Is that functionality so important that we can not release 6.0 
without it? I don't think so...

I really think it is not a good idea to push a "refactoring" patch if we already 
know that we need to refactor it again. Any refactoring should move our code in 
the direction we want it to be, not the opposite one! I can understand 
exceptions to that rule for fixing serious bugs or implementing must-have 
features. But WL#4296 doesn't fall into that category IMO.

I think we should accept the fact that a task which originally seemed to be 
simple, turned out to touch many aspects of our system design which we didn't 
think about before. Instead of hastily push a half-backed solution, we should 
sit down and think over these aspects of the design. Then we can use this task 
as a test-drive for the new design solutions. We can afford that, because we 
have a working logging mechanism which should be good enough for the first release.

I can agree that all I said above doesn't apply to the --backupdir option 
because it is a must-have feature. This is yet another good reason to separate 
this subtask. Then I would be more happy to accept an implementation even if it 
  is not fully consistent with the design ideas we have, because I'd understand 
that we must implement --backupdir for 6.0 release. Again, accepting a 
"temporary" patch for a task which says "Implement --backupdir option" is quite 
different from accepting such a patch for "Refine something" task.

Rafal

Chuck Bell wrote:
> Rafal,
> 
> Thanks for your very detailed review. I wasn't expecting such an effort and
> I am amazed you took the time to go through it in such fine detail -- above
> and beyond in my book (that's a compliment).
> 
> Just a few comments. I haven't had time to digest your entire treatise but I
> felt we need to discuss some fundamentals nonetheless. 
> 
>> Backup kernel/server separation
>> -------------------------------
> I understand your approach here and I don't disagree at a fundamental level.
> However, we had agreed earlier (I don't have it in writing as it were) that
> we were going to go down the KISS path for the logging in this worklog and
> that enhancements to the interface and usage would be separate tasks. The
> reason is we wanted to satisfy the architects' concerns first with the basic
> functionality before launching into a more detailed approach. 
> 
> If we launch with a complex design, I fear a scenario like what is occurring
> with si_objects. If we change how we do logging but don't change the rest of
> the server at the same time it may confuse people and create unnecessary
> debate. Perhaps this is unfounded, but that's how I see it.
> 
>> The backupdir option
>> --------------------
>>
>> I don't like that you included implementation of that option 
>> as part of WL#4296. 
>> Creating a monster task which includes several loosely 
>> related subtasks makes 
>> implementing and reviewing the changes much harder. I think 
>> "divide and conquer" 
>> is the best strategy here.
> 
> I am sorry you don't like including this here, but it was specifically
> requested by Peter. I needed to implement backupdir in order to get the logs
> to work properly. However, I have since figured out a way to separate the
> code so I am willing to work with you and make it a separate patch. I don't
> think that part would be a problem. However, it does need to be included
> with this work.
> 
> As for how it interacts with the backup code, I must again caution that we
> need to approach this in increments. Let's not try to design the minutia in
> the first pass. I think that mentality gets us into hot water too quickly
> (admittedly the opposite is equally bad). Rather, I'd like to see us get it
> working now so that later we have a baseline of functionality with which we
> can base improvements (not to mention mature) rather than trying to hit the
> final target on the first go. I think you and I will always agree to
> disagree on this point.
> 
>> Server installation
>> ------------------
>> When new server is installed, the backup logging tables 
>> should be created inside 
>> mysql database. I don't see that your patch provides code for that.
> 
> I will look into that. 
> 
> I think we need to decide now if I am going to implement your suggestions
> for redesign. Please think about this some more before saying 'no -- it must
> be my way' because we are only so many people and this work is on the list
> of things that need to be done now. My time is even more constricted this
> month and as it stands I will be lucky to have time to look at the code next
> week. It is unfortunate that we did not have more time to plan this work in
> more detail, but thems the breaks as they say.
> 
> What I propose is to make a new worklog detailing your ideas and class
> designs as well as other design elements you have highlighted. In this way,
> I think we can meet the goal of getting this work done and yet still have a
> purpose driven goal of getting it in the desired state. Otherwise, I
> honestly can't see this work getting done in July or even August (check the
> leave charts). I feel I must add that we do not really need this level of
> separation at this point. While I will agree we need to be conscious of the
> need to separate, I feel that the code I have written does not hinder us
> from doing so in the future (as is evident in your example patches).
> 
> Therefore, I ask you to please reconsider your detailed design changes.
> 
> Note: Many of the items regarding memory and such are things that are
> inherent in the code now. A lot of the code for the variables, backupdir,
> etc. are literally translations of existing code. I don't say this as an
> excuse or substitute for quality, rather I say this as way of explanation
> for how/why some of the code is written. I see no problems in making most of
> the improvements but I reserve the right to reject some based on precedence
> and conformity.
> 
> Chuck
> 
>