artag...@apache.org wrote on Sat, Sep 18, 2010 at 17:54:50 -0000:
> Author: artagnon
> Date: Sat Sep 18 17:54:50 2010
> New Revision: 998502
>
> URL: http://svn.apache.org/viewvc?rev=998502&view=rev
> Log:
> * subversion/svnrdump/load_editor.c: Attempt to acquire a lock of
> sorts before attempting to load. This also ensures that
> pre-revprop-change is enabled before it's too late.
>
> (get_lock): Add new function based on the corresponding function in
> svnsync/main.c. The locking operation can be described as attempting
> to change a dummy revprop in a delay-loop.
>
> (new_revision_record): Before doing anything else, call get_lock and
> release it at the end of the operations.
>
>
> Modified:
> subversion/trunk/subversion/svnrdump/load_editor.c
>
> Modified: subversion/trunk/subversion/svnrdump/load_editor.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/svnrdump/load_editor.c?rev=998502&r1=998501&r2=998502&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/svnrdump/load_editor.c (original)
> +++ subversion/trunk/subversion/svnrdump/load_editor.c Sat Sep 18 17:54:50
> 2010
> @@ -30,9 +30,15 @@
> #include "svn_path.h"
> #include "svn_ra.h"
> #include "svn_io.h"
> +#include "svn_private_config.h"
> +
> +#include <apr_network_io.h>
>
> #include "load_editor.h"
>
> +#define SVNRDUMP_PROP_LOCK "rdump-lock"
Need SVN_PROP_PREFIX.
> +#define LOCK_RETRIES 10
> +
> #ifdef SVN_DEBUG
> #define LDR_DBG(x) SVN_DBG(x)
> #else
> @@ -50,6 +56,58 @@ commit_callback(const svn_commit_info_t
> return SVN_NO_ERROR;
> }
>
> +static svn_error_t *
> +get_lock(svn_ra_session_t *session, apr_pool_t *pool)
> +{
> + char hostname_str[APRMAXHOSTLEN + 1] = { 0 };
> + svn_string_t *mylocktoken, *reposlocktoken;
> + apr_status_t apr_err;
> + apr_pool_t *subpool;
> + int i;
> +
> + apr_err = apr_gethostname(hostname_str, sizeof(hostname_str), pool);
> + if (apr_err)
> + return svn_error_wrap_apr(apr_err, _("Can't get local hostname"));
> +
> + mylocktoken = svn_string_createf(pool, "%s:%s", hostname_str,
> + svn_uuid_generate(pool));
> +
> + subpool = svn_pool_create(pool);
> +
> + for (i = 0; i < LOCK_RETRIES; ++i)
> + {
> + svn_pool_clear(subpool);
> +
> + SVN_ERR(svn_ra_rev_prop(session, 0, SVNRDUMP_PROP_LOCK,
> &reposlocktoken,
> + subpool));
> +
> + if (reposlocktoken)
> + {
> + /* Did we get it? If so, we're done, otherwise we sleep. */
> + if (strcmp(reposlocktoken->data, mylocktoken->data) == 0)
> + return SVN_NO_ERROR;
> + else
> + {
> + SVN_ERR(svn_cmdline_printf
> + (pool, _("Failed to get lock on destination "
> + "repos, currently held by '%s'\n"),
> + reposlocktoken->data));
> +
> + apr_sleep(apr_time_from_sec(1));
> + }
> + }
> + else if (i < LOCK_RETRIES - 1)
> + {
> + /* Except in the very last iteration, try to set the lock. */
> + SVN_ERR(svn_ra_change_rev_prop(session, 0, SVNRDUMP_PROP_LOCK,
> + mylocktoken, subpool));
> + }
> + }
> +
> + return svn_error_createf(APR_EINVAL, NULL,
> + _("Couldn't get lock on destination repos "
> + "after %d attempts"), i);
> +}
1. Please don't duplicate code.
2. If you do duplicate code, then add big comments (in all instances of
the duplication) pointing to the other instances.
3. Incidentally, I have modified the svnsync instance of this function
on the atomic-revprops branch. So the desire to avoid duplication isn't
just theoretical in this case...
>
> static svn_error_t *
> new_revision_record(void **revision_baton,
> @@ -539,13 +597,11 @@ drive_dumpstream_loader(svn_stream_t *st
> struct parse_baton *pb;
> pb = parse_baton;
>
> - /* ### TODO: Figure out if we're allowed to set revprops before
> - ### we're too late and mess up the repository. svnsync uses some
> - ### sort of locking mechanism. */
> -
> + SVN_ERR(get_lock(session, pool));
> SVN_ERR(svn_ra_get_repos_root2(session, &(pb->root_url), pool));
> SVN_ERR(svn_repos_parse_dumpstream2(stream, parser, parse_baton,
> NULL, NULL, pool));
> + SVN_ERR(svn_ra_change_rev_prop(session, 0, SVNRDUMP_PROP_LOCK, NULL,
> pool));
1. You can't change a property which isn't in the svn:* namespace.
2. Isn't svn_repos_parse_dumpstream2() an expensive function? You could
do the revprop test before that, in order to (if you have to fail) fail
sooner.
>
> return SVN_NO_ERROR;
> }
>
>
