[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1619413 - in /subversion/trunk/subversion: include/ libsvn_fs_fs/ libsvn_ra_local/ svnadmin/ svnlook/ tests/libsvn_fs_fs/

From: Evgeny Kotkov <evgeny.kotkov_at_visualsvn.com>
Date: Thu, 21 Aug 2014 19:59:08 +0400

> Author: stefan2
> Date: Thu Aug 21 14:35:05 2014
> New Revision: 1619413
> URL: http://svn.apache.org/r1619413
> Log:
> Enforce the "everybody or nobody" restriction on revprop caching, i.e.
> either all processes (usually servers) or non must use revprop caching.
> This patch makes sure that the first process to use revprop caching will
> create the revprop generation file before even reading and caching any
> revprops - instead of auto-creating it upon the first write. Then this
> file is being used as an indicator that the repo has been accessed at
> least once using revprop caching. FS instances without that feature
> enabled will then be banned from writing any revprops.

Is this a right thing to do? I suspect there are several problems:

- Any *existing* caller of our public API, namely svn_repos_open3(),
  svn_fs_open2() and their deprecated versions, will not be able to change
  revision properties for repositories that had this feature enabled at
  least once (!). We are not the only ones using that API, so should
  we tell them everyone to update their software depending on it if they
  would like it to work?

- The server now dictates the *caching* configuration for 'svn' and 'svnlook'
  clients. This looks problematic to me, especially considering some other
  problems stated in http://svn.haxx.se/dev/archive-2014-08/0235.shtml
  Shouldn't caching be totally optional by its design?

- How should existing administrators turn this feature off after they had it
  enabled for a while? With a change like this they cannot really just go
  with something like SVNCacheRevProps=off, because doing this would result
  in any attempt of changing a revision property to error out with "Writing
  revprops requires revprop caching to be enabled". Do they have to
  manually remove all the corresponding revprop generation files we now
  use as a marker? What if they have 60000 repositories? And how should
  they know about this in the first place?

- Finally, why cannot we just stop sharing the revprop generation data
  (as in http://svn.haxx.se/dev/archive-2014-08/0237.shtml)?

> +static svn_error_t *
> +enforce_consistent_revprop_caching(const svn_test_opts_t *opts,
> + apr_pool_t *pool)
> +{
> + svn_fs_t *fs_cache;
> + svn_fs_t *fs_no_cache;
> + svn_fs_t *fs_auto_cache;
> + apr_hash_t *fs_config = apr_hash_make(pool);
> + svn_string_t value = { "0", 1 };
> +
> + if (strcmp(opts->fs_type, "fsfs") != 0)
> + return svn_error_create(SVN_ERR_TEST_SKIPPED, NULL, NULL);
> +
> + /* Create test repository with greek tree. */
> + SVN_ERR(svn_test__create_fs(&fs_no_cache, REPO_NAME, opts, pool));
> +
> + svn_hash_sets(fs_config, SVN_FS_CONFIG_FSFS_CACHE_REVPROPS, "1");
> + SVN_ERR(svn_fs_open2(&fs_cache, REPO_NAME, fs_config, pool, pool));
> +
> + svn_hash_sets(fs_config, SVN_FS_CONFIG_FSFS_CACHE_REVPROPS, "3");
> + SVN_ERR(svn_fs_open2(&fs_auto_cache, REPO_NAME, fs_config, pool, pool));

I suspect that the first svn_fs_open2() requires a warning handle. Otherwise,
this test would abort on platforms with inefficient named atomics.

> /* The test table. */
> @@ -1269,7 +1336,10 @@ static struct svn_test_descriptor_t test
> SVN_TEST_OPTS_WIMP(revprop_caching_on_off,
> "change revprops with enabled and disabled caching",
> "fails with FSFS / svn_named_atomic__is_efficient()"),
> + SVN_TEST_OPTS_PASS(enforce_consistent_revprop_caching,
> + "test revprop cache setting consistency"),
> };
> +
> \ No newline at end of file

Shouldn't we now make the revprop_caching_on_off() a SVN_TEST_OPTS_PASS()?

Evgeny Kotkov
Received on 2014-08-21 17:59:55 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.