RE: bk commit into 6.0 tree (cbell:1.2784) BUG#33562

[Chuck Bell]

Rafal,

I understand the arguments and you really don't need to defend them. If
given a vote as to how to proceed, I would still stick with my patch as the
simplest means to solve the immediate problem and delay your changes until
such time as we design the asynchronous execution code -- there are too many
unknowns WRT asynchronous behavior to warrant changing the logger class now.
What concerns me most is the changes that may be forced upon the error
handling code which will change the logger class well beyond what it is now.
Why implement a decisively diverse patch now when it is likely more changes
will be needed in the future? I favor small changes that do not affect
existing functionality over wider spread changes that add nothing to the
existing functionality. Leave redesign for redesign efforts.

Chuck 

> -----Original Message-----
> From: Rafal Somla [mailto:rsomla@stripped] 
> Sent: Monday, January 28, 2008 10:52 AM
> To: Chuck Bell
> Cc: commits@stripped; 'Backup development list'
> Subject: Re: bk commit into 6.0 tree (cbell:1.2784) BUG#33562
> 
> Chuck,
> 
> You basically agreed to go for my patch but I would still 
> like to know that you understand my reasons behind the 
> different solution.
> 
> Chuck Bell wrote:
> > I don't understand: "a) and b), as they are not (will be not) 
> > available during the process" -- how is that possible? Even if we 
> > decouple the logger class from the server we will still need to 
> > communicate with the server's error handling code.
> 
> This is the main problem I'm trying to solve. Destinations a) 
> and b) are the error code/message returned from 
> BACKUP/RESTORE command and client's error/warning stack which 
> is populated while server executes client's SQL statement.
> 
> When BACKUP/RESTORE is asynchronous, these destinations are 
> not available for the whole duration of the backup/restore 
> process. Imagine this: DBA connects to server and issues 
> "BACKUP DATABASE foo TO 'foo.bak'". The statement returns 
> immediately and successfully, returning backup id. Then DBA 
> can even close his connection to the server and the backup 
> operation will continue in the background. Thus, obviously, 
> any errors or warnings issued while backup is running can not 
> be reported as a result of the BACKUP statement (which has 
> already returned) or pushed on the client's error/warning 
> stack (as client has disconnected).
> 
> This is why the Logger class should not report errors via 
> my_error() or
> push_warning() calls - the logger instance will exists during 
> whole time of backup/restore operation and will be used to 
> report errors/warnings at any moment - possibly also when 
> there is no client connection any more.
> 
> For this reason, I want to separate generating the result of 
> BACKUP/RESTORE statements (whether it is an error or a valid 
> backup_id result) from logging backup related error/warnings. 
> The idea is that the backup logging mechanism, in the form of 
> the Logger class, will be designed as we want it to work in 
> the future, while handling of BACKUP/RESTORE result might 
> change as needed.
> 
> For example, now the BACKUP/RESTORE are synchronous and 
> therefore they can and should report all errors/warning 
> reported during the operation. But in the future they will be 
> asynchronous and thus will not do it any more. I want to 
> change the way BACKUP/RESTORE report errors, but not the way 
> Logger works.
> 
> And this is what my patch does. It assumes that Logger works 
> as it will work in the future, that is, not report errors to 
> the client. Instead, all reported errors and warnings are 
> stored inside the logger and then, when the backup/restore 
> operation is finished, the code which is responsible for 
> sending response to the client (i.e. execute_backup_command() 
> function) iterates over stored errors/warnings, pushes them 
> on the client's stack and returns error code/message if appropriate.
> 
> Rafal
> 
> I think this abstraction is too complex and we need not
> > go so far. What is the goal of such complexity? What 
> precept of OOD are we
> > trying to achieve? Devil's advocate question: Why should a 
> logger for online
> > backup not log warnings and errors and pass them to the server?
> > 
> > </commentary>
> > 
> > Chuck  
> >