On Wed, Jun 3, 2015 at 2:33 PM, Branko Čibej <brane_at_wandisco.com> wrote:
> On 21.05.2015 17:23, Evgeny Kotkov wrote:
> > Subversion 1.9.0-rc1 introduced a new svnadmin verify --keep-going mode
> [1].
> > In order to achieve this, we added a svn_repos_verify_fs3() API function
> and
> > deprecated its predecessor, svn_repos_verify_fs2(), that now calls the
> newer
> > function with keep_going argument set to FALSE.
> >
> > However, svn_repos_verify_fs2() behaves differently in 1.9.0-rc1 and
> 1.8.13
> > when it comes to error reporting. As an example, the following code ...
> >
> > SVN_ERR(svn_repos_verify_fs2(repos, 1, 5, NULL, NULL, NULL, NULL,
> pool));
> >
> > ...would return two different errors depending on the binaries being
> used,
> > assuming that one of the revisions in the [r1:r5] range is corrupted:
> >
> > (With 1.8.13) E160004: Checksum mismatch in item at offset 0 of
> > length 59 bytes in file path/asf/db/revs/0/2
> >
> > (With 1.9.0-rc1) E165011: Repository 'path/asf' failed to verify
> >
> > Please note that the second error is generic, and that the actual
> information
> > about the error is lost. Existing API users of svn_repos_verify_fs2()
> are
> > going to lose an important bit of information about the root cause of the
> > corruption unless they update the code. Furthermore, even if an API
> caller
> > subscribes to notifications with a notify_func / notify_baton pair, it
> would
> > still be necessary to update the code to handle svn_repos_notify_failure
> that
> > did not exist before 1.9.
> >
> > I did not find any discussion on the matter or the corresponding entry in
> > /notes/api-errata [2] that would describe this behavior change, so I am
> > inclined to think that this is accidental and, probably, undesirable.
> >
> > There is an option of restoring the 1.8 behavior when
> svn_repos_verify_fs3()
> > is called with keep_going set to FALSE. We could immediately yield the
> error
> > in this case, and only use the notifications / generic error when
> keep_going
> > is set to TRUE. Doing so would change the output of svnadmin verify
> without
> > --keep-going, because now it wouldn't include the generic error message
> in
> > the end. I attached a proof-of-concept patch, that doesn't change the
> test
> > expectations and the documentation, i.e., it only includes the core
> change,
> > mostly as an illustration to these points.
> >
> > Thoughts?
> >
> > [1] https://svn.apache.org/r1492651
> > [2]
> https://svn.apache.org/repos/asf/subversion/trunk/notes/api-errata/1.9
>
> I completed your patch and committed the fix in r1683311. Please review!
>
Hi Evgeny & Brane,
r1683311 looks good and I'll approve it for 1.9.x in a minute.
The reason why the FS backend returned an REPOS error
is its suggestive name. After all, the repos is corrupt ...
But that temptation has been fixed as well now.
On a more meta note concerning repos verification, we aren't
too thorough in what we test and errors that are reported often
don't give the admin a clue on how they might fix things. For
the first part, it would be nice to be able to test that a repo is
writeable. The second part probably needs planning / a concept
of which kind of problems we test for explicitly, e.g. missing
or superfluous files. Our "lets try to read all data" stage would
then be only the last step in the process.
Nothing urgent but maybe something to talk about in Berlin.
-- Stefan^2.
Received on 2015-06-04 14:43:42 CEST