On 3 June 2015 at 15:33, Branko Čibej <br...@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 <br...@wandisco.com> wrote: > On 03.06.2015 14:57, Ivan Zhakov wrote: >> On 3 June 2015 at 15:35, Branko Čibej <br...@wandisco.com> wrote: >>> On 03.06.2015 14:24, Ivan Zhakov wrote: >>>> On 3 June 2015 at 14:37, Branko Čibej <br...@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
