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. I'm also not sure that we have to
return error if we already reported it via notify_func in
svn_repos_verify_fs3().
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Received on 2015-06-03 15:32:04 CEST