On 3 June 2015 at 17:19, Branko Čibej <brane_at_wandisco.com> wrote:
> On 03.06.2015 15:31, Ivan Zhakov wrote:
>
> On 3 June 2015 at 15:33, 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!
>
> [returning discussion back to this thread]
>
> On 3 June 2015 at 16:26, Branko Čibej <brane_at_wandisco.com> wrote:
>
> On 03.06.2015 14:57, Ivan Zhakov wrote:
>
> On 3 June 2015 at 15:35, Branko Čibej <brane_at_wandisco.com> wrote:
>
> On 03.06.2015 14:24, Ivan Zhakov wrote:
>
> On 3 June 2015 at 14:37, Branko Čibej <brane_at_wandisco.com> wrote:
>
> On 02.06.2015 20:05, Branko Čibej wrote:
>
> On 02.06.2015 12:45, Daniel Shahaf wrote:
>
> Ben Reser wrote on Sun, May 31, 2015 at 14:28:39 -0700:
>
> The 1.9.0-rc2 release artifacts are now available for testing/signing.
> Please get the tarballs from
> https://dist.apache.org/repos/dist/dev/subversion
> and add your signatures there.
>
> Thanks!
>
> Note that Evgeny reported a regression in svn_repos_verify_fs2() in
> <http://svn.haxx.se/dev/archive-2015-05/0141.shtml>. No objections to
> moving forward with rc2, but as that issue is a regression, we'll need
> an rc3 that fixes it.
>
> Yes ... and the patch has been reviewed but not committed. I believe it
> only needs a couple tweaks (fixing an "if" condition and removing the
> unused error code).
>
> I have a more complete fix based on Evgeny's patch, running tests now.
> It turns out that we still need a new error code for the summary
> results, but with a different meaning and therefore different name.
> Renaming it had a positive side effect as it turned out that we were
> emitting the SVN_ERR_REPOS_CORRUPTED error from FSFS and FSX instead of
> the correct SVN_ERR_FS_CORRUPT.
>
> Another option would be to require notify_func for
> svn_repos_verify_fs3() and always report errors through notify_func.
> This would make error reporting consistent whether keep_going TRUE or
> FALSE. For svn_repos_verify_fs2() we could create compat notify_func
> handler that converts notifications to errors.
>
> See r1683311; I believe error reporting is consistent now.
> Adding complex logic to the deprecated function isn't such a good idea, IMO.
>
> Maybe, but it's better than adding complex logic to current
> (not-deprecated) function. IMO it's not svn_repos_verify_fs3()
> responsibility to generate verification summary -- it should be done
> at the UI level. I.e. in svnadmin or other application using
> svn_repos_verify_fs3() API.
>
> But that would imply either returning a bunch of extra info from
> svn_repos_verify_fs3, or counting notifications in the callers (which
> means making the callers depend on implementation details of the API).
>
> I don't see problem provide wrapped notify func in
> svn_repos_verify_fs2() in call to svn_repos_verify_fs3(). Also we
> don't need to count notifications since svn_repos_verify_fs2() doesn't
> support keep_going flag.
>
> Note that the summary will only be generated if an error occurred during
> verification in keep-going mode, when we have to return some kind of
> error anyway; we may as well put as much info as we have available into
> the error message.
>
> That's what I call inconsistent API: the returned errors are different
> whether keep_going TRUE or FALSE.
>
>
> Of course the errors are different, the mode of operation is different, too.
> Keep-going mode drops underlying errors on purpose (after notifying); it's
> been this way from the beginning, it makes perfect sense and I didn't change
> that. What Evgeny and I fixed was the bug that the keep_going=FALSE mode
> also dropped the underlying errors, which is both wrong and inconsistent
> with 1.8.x. (And that keep_going=TRUE mode ignored cancellations.)
>
>> I'm also not sure that we have to return error if we already reported it via notify_func in
>> svn_repos_verify_fs3().
>
>
> Out notification mechanism cannot replace API consistency. When an API call
> fails, it must return an svn_error_t; surely you're not proposing that we
> make an exception for svn_repos_verify_fs3?
>
This is depends whether check of corrupted repository is error or not.
I mean that semantic could be: "please give me all/first corruptions
in this repository via notify_func".
But I'm not insisting on this approach.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Received on 2015-06-03 17:11:29 CEST