You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2010/09/19 09:29:58 UTC
Re: svn commit: r998502 -
/subversion/trunk/subversion/svnrdump/load_editor.c
artagnon@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;
> }
>
>
Re: svn commit: r998502 -
/subversion/trunk/subversion/svnrdump/load_editor.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Ramkumar Ramachandra wrote on Mon, Sep 20, 2010 at 10:35:34 +0530:
> Hi Daniel,
>
> Daniel Shahaf writes:
> > Ramkumar Ramachandra wrote on Sun, Sep 19, 2010 at 16:52:50 +0530:
> > > Daniel Shahaf writes:
> > > > Stefan Sperling wrote on Sun, Sep 19, 2010 at 11:40:49 +0200:
> > > > > On Sun, Sep 19, 2010 at 10:29:58AM +0100, Daniel Shahaf wrote:
> > > > > > 2. If you do duplicate code, then add big comments (in all instances of
> > > > > > the duplication) pointing to the other instances.
> > > > >
> > > > > +1
> > >
> > > I've mentioned it in the commit message, but alright- I'll add a
> > > comment in the file.
> > >
> >
> > Please add a comment to svnsync's get_lock() as well.
>
> +1 needed :)
>
+1 given, but please s/##/###/ for consistency :)
> [[[
> * subversion/svnsync/main.c
> (get_lock): Add a comment explaining what the function does along
> with a note about it being duplicated in svnrdump.
>
> * subversion/svnrdump/load_editor.c
> (get_lock): Add a comment explaining what the function does along
> with a note about it being duplicated in svnsync.
> ]]]
>
> Index: subversion/svnsync/main.c
> ===================================================================
> --- subversion/svnsync/main.c (revision 998652)
> +++ subversion/svnsync/main.c (working copy)
> @@ -285,7 +285,13 @@ check_lib_versions(void)
>
>
> /* Acquire a lock (of sorts) on the repository associated with the
> - * given RA SESSION.
> + * given RA SESSION. This lock is just a revprop change attempt in a
> + * time-delay loop. This function is duplicated by svnrdump in
> + * load_editor.c.
> + *
> + * ## TODO: Make this function more generic and
> + * expose it through a header for use by other Subversion
> + * applications to avoid duplication.
> */
> static svn_error_t *
> get_lock(svn_ra_session_t *session, apr_pool_t *pool)
> Index: subversion/svnrdump/load_editor.c
> ===================================================================
> --- subversion/svnrdump/load_editor.c (revision 998772)
> +++ subversion/svnrdump/load_editor.c (working copy)
> @@ -56,6 +56,14 @@ commit_callback(const svn_commit_info_t *commit_in
> return SVN_NO_ERROR;
> }
>
> +/* Acquire a lock (of sorts) on the repository associated with the
> + * given RA SESSION. This lock is just a revprop change attempt in a
> + * time-delay loop. This function is duplicated by svnsync in main.c.
> + *
> + * ## TODO: Make this function more generic and
> + * expose it through a header for use by other Subversion
> + * applications to avoid duplication.
> + */
> static svn_error_t *
> get_lock(svn_ra_session_t *session, apr_pool_t *pool)
> {
>
> > Thanks for the offer. I can do that myself, though; svnrdump is
> > a first-class citizen now, as is this duplication (per the direction
> > this thread seems to be going), so AFAICT all cards say the branch
> > maintainer should be adding that to svnrdump.
>
> Sure :)
>
> -- Ram
Re: svn commit: r998502 -
/subversion/trunk/subversion/svnrdump/load_editor.c
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Daniel,
Daniel Shahaf writes:
> Ramkumar Ramachandra wrote on Sun, Sep 19, 2010 at 16:52:50 +0530:
> > Daniel Shahaf writes:
> > > Stefan Sperling wrote on Sun, Sep 19, 2010 at 11:40:49 +0200:
> > > > On Sun, Sep 19, 2010 at 10:29:58AM +0100, Daniel Shahaf wrote:
> > > > > 2. If you do duplicate code, then add big comments (in all instances of
> > > > > the duplication) pointing to the other instances.
> > > >
> > > > +1
> >
> > I've mentioned it in the commit message, but alright- I'll add a
> > comment in the file.
> >
>
> Please add a comment to svnsync's get_lock() as well.
+1 needed :)
[[[
* subversion/svnsync/main.c
(get_lock): Add a comment explaining what the function does along
with a note about it being duplicated in svnrdump.
* subversion/svnrdump/load_editor.c
(get_lock): Add a comment explaining what the function does along
with a note about it being duplicated in svnsync.
]]]
Index: subversion/svnsync/main.c
===================================================================
--- subversion/svnsync/main.c (revision 998652)
+++ subversion/svnsync/main.c (working copy)
@@ -285,7 +285,13 @@ check_lib_versions(void)
/* Acquire a lock (of sorts) on the repository associated with the
- * given RA SESSION.
+ * given RA SESSION. This lock is just a revprop change attempt in a
+ * time-delay loop. This function is duplicated by svnrdump in
+ * load_editor.c.
+ *
+ * ## TODO: Make this function more generic and
+ * expose it through a header for use by other Subversion
+ * applications to avoid duplication.
*/
static svn_error_t *
get_lock(svn_ra_session_t *session, apr_pool_t *pool)
Index: subversion/svnrdump/load_editor.c
===================================================================
--- subversion/svnrdump/load_editor.c (revision 998772)
+++ subversion/svnrdump/load_editor.c (working copy)
@@ -56,6 +56,14 @@ commit_callback(const svn_commit_info_t *commit_in
return SVN_NO_ERROR;
}
+/* Acquire a lock (of sorts) on the repository associated with the
+ * given RA SESSION. This lock is just a revprop change attempt in a
+ * time-delay loop. This function is duplicated by svnsync in main.c.
+ *
+ * ## TODO: Make this function more generic and
+ * expose it through a header for use by other Subversion
+ * applications to avoid duplication.
+ */
static svn_error_t *
get_lock(svn_ra_session_t *session, apr_pool_t *pool)
{
> Thanks for the offer. I can do that myself, though; svnrdump is
> a first-class citizen now, as is this duplication (per the direction
> this thread seems to be going), so AFAICT all cards say the branch
> maintainer should be adding that to svnrdump.
Sure :)
-- Ram
Re: svn commit: r998502 -
/subversion/trunk/subversion/svnrdump/load_editor.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Ramkumar Ramachandra wrote on Sun, Sep 19, 2010 at 16:52:50 +0530:
> Daniel Shahaf writes:
> > Stefan Sperling wrote on Sun, Sep 19, 2010 at 11:40:49 +0200:
> > > On Sun, Sep 19, 2010 at 10:29:58AM +0100, Daniel Shahaf wrote:
> > > > 2. If you do duplicate code, then add big comments (in all instances of
> > > > the duplication) pointing to the other instances.
> > >
> > > +1
>
> I've mentioned it in the commit message, but alright- I'll add a
> comment in the file.
>
Please add a comment to svnsync's get_lock() as well.
> > > > 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...
> > >
> > > We should fix the race in svnrdump on the atomic-revprop branch as well.
> > >
> >
> > No problem; r998622. (The svnsync patch isn't committed yet, but it
> > should be easy to port it to svnrdump.)
>
> Ok, let me know when it's done. I'll port it to svnrdump too.
Thanks for the offer. I can do that myself, though; svnrdump is
a first-class citizen now, as is this duplication (per the direction
this thread seems to be going), so AFAICT all cards say the branch
maintainer should be adding that to svnrdump.
Daniel
Re: svn commit: r998502 -
/subversion/trunk/subversion/svnrdump/load_editor.c
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi,
Daniel Shahaf writes:
> Stefan Sperling wrote on Sun, Sep 19, 2010 at 11:40:49 +0200:
> > On Sun, Sep 19, 2010 at 10:29:58AM +0100, Daniel Shahaf wrote:
> > > 1. Please don't duplicate code.
> >
> > I think it's fine for svnrdump to have its own copy of this for now.
> > We could at some point merge them into libsvn_subr/ of course, but I
> > don't see a huge problem with just 2 instances.
Ok, let's have this copy for now.
> > > 2. If you do duplicate code, then add big comments (in all instances of
> > > the duplication) pointing to the other instances.
> >
> > +1
I've mentioned it in the commit message, but alright- I'll add a
comment in the file.
> > > 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...
> >
> > We should fix the race in svnrdump on the atomic-revprop branch as well.
> >
>
> No problem; r998622. (The svnsync patch isn't committed yet, but it
> should be easy to port it to svnrdump.)
Ok, let me know when it's done. I'll port it to svnrdump too.
-- Ram
Re: svn commit: r998502 -
/subversion/trunk/subversion/svnrdump/load_editor.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Sun, Sep 19, 2010 at 11:40:49 +0200:
> On Sun, Sep 19, 2010 at 10:29:58AM +0100, Daniel Shahaf wrote:
> > artagnon@apache.org wrote on Sat, Sep 18, 2010 at 17:54:50 -0000:
> > > +static svn_error_t *
> > > +get_lock(svn_ra_session_t *session, apr_pool_t *pool)
> > > +{
...
> > > +}
> >
> > 1. Please don't duplicate code.
>
> I think it's fine for svnrdump to have its own copy of this for now.
> We could at some point merge them into libsvn_subr/ of course, but I
> don't see a huge problem with just 2 instances.
>
> > 2. If you do duplicate code, then add big comments (in all instances of
> > the duplication) pointing to the other instances.
>
> +1
>
> > 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...
>
> We should fix the race in svnrdump on the atomic-revprop branch as well.
>
No problem; r998622. (The svnsync patch isn't committed yet, but it
should be easy to port it to svnrdump.)
> Stefan
Re: svn commit: r998502 -
/subversion/trunk/subversion/svnrdump/load_editor.c
Posted by Stefan Sperling <st...@elego.de>.
On Sun, Sep 19, 2010 at 10:29:58AM +0100, Daniel Shahaf wrote:
> artagnon@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.
> > +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.
I think it's fine for svnrdump to have its own copy of this for now.
We could at some point merge them into libsvn_subr/ of course, but I
don't see a huge problem with just 2 instances.
> 2. If you do duplicate code, then add big comments (in all instances of
> the duplication) pointing to the other instances.
+1
> 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...
We should fix the race in svnrdump on the atomic-revprop branch as well.
Stefan
Re: svn commit: r998502 -
/subversion/trunk/subversion/svnrdump/load_editor.c
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Daniel,
Daniel Shahaf writes:
> artagnon@apache.org wrote on Sat, Sep 18, 2010 at 17:54:50 -0000:
> > +#define SVNRDUMP_PROP_LOCK "rdump-lock"
>
> Need SVN_PROP_PREFIX.
Fixed in r998772.
> 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...
Discussed in another email.
> > - /* ### 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.
Fixed in r998772.
> 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.
Oh, the last svn_ra_change_rev_prop is only meant to release the
lock. I do the revprop test in the first line: see get_lock.
Thanks for the review.
-- Ram