Re: svn commit: r1394345 - /subversion/trunk/subversion/libsvn_fs_fs/caching.c

On Fri, Oct 5, 2012 at 10:14 AM, Bert Huijben <bert_at_qqmail.nl> wrote:

>
> > -----Original Message-----
> > From: stefan2_at_apache.org [mailto:stefan2_at_apache.org]
> > Sent: vrijdag 5 oktober 2012 03:54
> > To: commits_at_subversion.apache.org
> > Subject: svn commit: r1394345 -
> > /subversion/trunk/subversion/libsvn_fs_fs/caching.c
> >
> > Author: stefan2
> > Date: Fri Oct 5 01:54:12 2012
> > New Revision: 1394345
> >
> > URL: http://svn.apache.org/viewvc?rev=1394345&view=rev
> > Log:
> > Since zero-copy code can cause cache functions to actually return
> > an error (lost connection etc.), we must make sure these get passed
> > through to the calling functions.
> >
> > * subversion/libsvn_fs_fs/caching.c
> > (warn_on_cache_errors): don't swallow the error, just log it
> >
> > Modified:
> > subversion/trunk/subversion/libsvn_fs_fs/caching.c
> >
> > Modified: subversion/trunk/subversion/libsvn_fs_fs/caching.c
> > URL:
> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/ca
> > ching.c?rev=1394345&r1=1394344&r2=1394345&view=diff
> > ==========================================================
> > ====================
> > --- subversion/trunk/subversion/libsvn_fs_fs/caching.c (original)
> > +++ subversion/trunk/subversion/libsvn_fs_fs/caching.c Fri Oct 5
> 01:54:12
> > 2012
> > @@ -103,8 +103,7 @@ warn_on_cache_errors(svn_error_t *err,
> > {
> > svn_fs_t *fs = baton;
> > (fs->warning)(fs->warning_baton, err);
> > - svn_error_clear(err);
> > - return SVN_NO_ERROR;
> > + return err;
> > }
>
> Usually warnings are non-fatal (/non errors). Maybe you should rename this
> function or handle specific errors?
>
>
svn_fs_t does not have an error callback but the things we
are logging here are real failures from various sources.
Yes, there is a mismatch.

Please note that this is a relatively old functionality
contributed by David Glasser in 2008. My guess is that he
wanted to log things like intermittent memcached failures.
The latter may now be fragile as all kinds of network issues
cancel the respective operation / report.

r1394470 should address this problem.

Thanks for the review!

-- Stefan^2.

-- 
*
Join us this October at Subversion Live
2012<http://www.wandisco.com/svn-live-2012>
 for two days of best practice SVN training, networking, live demos,
committer meet and greet, and more! Space is limited, so get signed up
today<http://www.wandisco.com/svn-live-2012>
!
*