On 13 April 2015 at 15:23, <rhuijben_at_apache.org> wrote:
> Author: rhuijben
> Date: Mon Apr 13 12:23:20 2015
> New Revision: 1673170
>
> URL: http://svn.apache.org/r1673170
> Log:
> Add an fs layer api that allows obtaining just a boolean indicating whether
> properties exist on a node, instead of always obtaining the properties and
> checking their count.
>
> This is by far the most expensive operation on 'svn ls -v' in Subversion <=
> 1.8.x on huge directories as it requires fetching much 'new' data, and has
> the risk of trashing the node cache.
>
> r1673153 made new 'svn' clients stop asking for this information for this
> scenario but existing clients do ask and so will most likely many third
> party clients (confirmed for TortoiseSVN), will keep asking for this
> information.
>
> This function introduces the FS api and updates callers, but doesn't provide
> optimized implementations yet, so the result is that this doesn't change
> runtime behaviour yet, but just moves the implementation into the fs layer.
>
> I hope this patch will be accepted for 1.9.0 to allow further improvements
> in later patches, potentially after 1.9.0.
>
Hi Bert,
It looks like a interesting optimization, but I've some concerns about
mod_dav_svn protocol part. See my review inline.
> Modified: subversion/trunk/subversion/mod_dav_svn/liveprops.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/liveprops.c?rev=1673170&r1=1673169&r2=1673170&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/mod_dav_svn/liveprops.c (original)
> +++ subversion/trunk/subversion/mod_dav_svn/liveprops.c Mon Apr 13 12:23:20 2015
> @@ -787,30 +787,58 @@ insert_prop_internal(const dav_resource
>
> case SVN_PROPID_deadprop_count:
> {
> - unsigned int propcount;
> - apr_hash_t *proplist;
> -
> if (resource->type != DAV_RESOURCE_TYPE_REGULAR)
> return DAV_PROP_INSERT_NOTSUPP;
>
> - serr = svn_fs_node_proplist(&proplist,
> - resource->info->root.root,
> - resource->info->repos_path, scratch_pool);
> - if (serr != NULL)
> + if (resource->info->repos->is_svn_client)
> {
> - ap_log_rerror(APLOG_MARK, APLOG_ERR, serr->apr_err,
> - resource->info->r,
> - "Can't fetch proplist of '%s': "
> - "%s",
> - resource->info->repos_path,
> - serr->message);
> - svn_error_clear(serr);
> - value = error_value;
> - break;
> + svn_boolean_t has_props;
> + /* Retrieving the actual properties is quite expensive while
> + svn clients only want to know if there are properties, and
> + in many cases aren't interested at all (see r1673153) */
> + serr = svn_fs_node_has_props(&has_props,
> + resource->info->root.root,
> + resource->info->repos_path,
> + scratch_pool);
> +
> + if (serr != NULL)
> + {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, serr->apr_err,
> + resource->info->r,
> + "Can't fetch has props of '%s': "
> + "%s",
> + resource->info->repos_path,
> + serr->message);
> + svn_error_clear(serr);
> + value = error_value;
> + break;
> + }
> +
> + value = has_props ? "99" /* Magic (undocumented) value */ : "0";
> }
> + else
> + {
> + unsigned int propcount;
> + apr_hash_t *proplist;
> + serr = svn_fs_node_proplist(&proplist,
> + resource->info->root.root,
> + resource->info->repos_path, scratch_pool);
> + if (serr != NULL)
> + {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, serr->apr_err,
> + resource->info->r,
> + "Can't fetch proplist of '%s': "
> + "%s",
> + resource->info->repos_path,
> + serr->message);
> + svn_error_clear(serr);
> + value = error_value;
> + break;
> + }
As far I understand the new server will return magic value (99) for
dead-prop-count DAV property for all Subversion clients. Is it
correct? It looks like hacky approach and it could break backward
compatibility. I'm aware that currently we don't use deadprop-count
other than check for non-zero, but I don't think we should change
server-side behavior anyway.
The proper solution would be add new DAV property like
"has-dead-props", advertise it support using capability header and
then request it from client instead of "deadprop-count".
What do you think?
--
Ivan Zhakov
Received on 2015-04-13 14:53:29 CEST