Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
[An update on my thinking here.] I have a patch in which svnrdump hard-codes an override of the runtime configuration to set two things: http-max-connections = 2 http-bulk-updates = TRUE The bulk updates thing will fix the known problems for servers which allow them (which really ought to be most live servers). If the server says, Sorry, no bulk updates, then the idea is that limiting ra_serf to a single aux connections will do the trick. Unfortunately, it doesn't do the trick right now, because even ensuring that all the GETs/PROPFINDs come down in the right order does not a well-ordered editor drive make. Because ra_serf still only closes directories opportunistically, it could process GETs for files across multiple subdirs before ever taking a moment to pause and see what needs to be cleaned up. That's not to say that improvements in svnrdump itself can't work around these remaining ordering problems. Someone (I think it was Bert...) recently changed svnrdump to actually use separate file and directory and editor batons, which was definitely a step in the right direction. And I believe there is still some separation which could be done, so maybe finishing that work gets us to where we need to be. So, I'm not raising a white flag -- I'm merely saying that right now I'm still observing brokenness even when ra_serf is only using a single aux connection. I'll return to debugging all this on Monday. -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Enterprise Cloud Development
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
On 12/13/2012 05:18 PM, Lieven Govaerts wrote: IMHO, the option to make ra_serf respect a more stricter interpretation of the editor api is something we need to give to a developer, not to a user via config. I get the gist of what you're saying, but don't agree with the specifics starting points and directions. It's not that we're asking ra_serf to use a stricter interpretation of the editor api -- it's that we're asking it to honor that API *at all*. All of these (and several other) problems with ra_serf stem for the simple fact that it was not written to obey the spec. We've dealt with it thus far by forcing our editor implementation to be able to gracefully handle ra_serf's shortcomings in this respect. So if there's an developer-accessible option to be had here, the default should be for the RA layer to be well-behaved, with the option to break the editor drive rules in some well-defined way. The choices should be two different known behaviors, not merely Order and Chaos. -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Enterprise Cloud Development signature.asc Description: OpenPGP digital signature
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
On Wed, Dec 12, 2012 at 4:24 PM, C. Michael Pilato cmpil...@collab.netwrote: Here's a question I've been wondering for some time: should we expose to users a configuration option for declaring the number of aux connections ra_serf should use? I mean, Firefox has exposed such an option for years. If we did so for Subversion, we could say, Yeah, ra_serf+svnrdump is a bad combination. Here's a workaround. Run it with --config-option=servers:global:serf-max-connections=2. Or better yet (as per my other mail in this thread), we could just hardcode that option override in svnrdump itself. Both approaches (allow config option to control # of conns; have svnrdump disable parallelism) seems reasonable to me. FWIW, I'd probably set it to 8 - since 2006, most browsers upped their connection parallelism to 8. *grin* And, yah, it's quite obvious from reading the thread that you missed a bunch of Lieven's emails. -- justin
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
On 12/13/2012 05:48 AM, Justin Erenkrantz wrote: On Wed, Dec 12, 2012 at 4:24 PM, C. Michael Pilato cmpil...@collab.net mailto:cmpil...@collab.net wrote: Here's a question I've been wondering for some time: should we expose to users a configuration option for declaring the number of aux connections ra_serf should use? I mean, Firefox has exposed such an option for years. If we did so for Subversion, we could say, Yeah, ra_serf+svnrdump is a bad combination. Here's a workaround. Run it with --config-option=servers:global:serf-max-connections=2. Or better yet (as per my other mail in this thread), we could just hardcode that option override in svnrdump itself. Both approaches (allow config option to control # of conns; have svnrdump disable parallelism) seems reasonable to me. FWIW, I'd probably set it to 8 - since 2006, most browsers upped their connection parallelism to 8. *grin* Okay. In r1421559 I implemented the http-max-connections option. Defaults to 4. Hard-coded limit of 8. (All that is up for discussion/debate/etc.) But on the way to adding the code to svnrdump to hard-code that configuration option, I found myself wondering ... could we get away with using a hardcoded-into-svnrdump *private* configuration variable that just means, Do whatever you need to ensure that the editor is driven sanely? I realize this is similar to the idea I previously shot down, but the difference here is that I'm not talking about using a user-visible, shows-up-in-the-config-template-file option here. Just a #define and a value stuffed into the configuration hash. Thoughts? -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Enterprise Cloud Development signature.asc Description: OpenPGP digital signature
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
Mike, On Thu, Dec 13, 2012 at 11:06 PM, C. Michael Pilato cmpil...@collab.net wrote: On 12/13/2012 05:48 AM, Justin Erenkrantz wrote: On Wed, Dec 12, 2012 at 4:24 PM, C. Michael Pilato cmpil...@collab.net mailto:cmpil...@collab.net wrote: Here's a question I've been wondering for some time: should we expose to users a configuration option for declaring the number of aux connections ra_serf should use? I mean, Firefox has exposed such an option for years. If we did so for Subversion, we could say, Yeah, ra_serf+svnrdump is a bad combination. Here's a workaround. Run it with --config-option=servers:global:serf-max-connections=2. Or better yet (as per my other mail in this thread), we could just hardcode that option override in svnrdump itself. Both approaches (allow config option to control # of conns; have svnrdump disable parallelism) seems reasonable to me. FWIW, I'd probably set it to 8 - since 2006, most browsers upped their connection parallelism to 8. *grin* Okay. In r1421559 I implemented the http-max-connections option. Defaults to 4. Hard-coded limit of 8. (All that is up for discussion/debate/etc.) But on the way to adding the code to svnrdump to hard-code that configuration option, I found myself wondering ... could we get away with using a hardcoded-into-svnrdump *private* configuration variable that just means, Do whatever you need to ensure that the editor is driven sanely? I realize this is similar to the idea I previously shot down, but the difference here is that I'm not talking about using a user-visible, shows-up-in-the-config-template-file option here. Just a #define and a value stuffed into the configuration hash. Thoughts? I have thought about the same thing before, but I don't think it's a good idea. It will give our applications sort of an extra API that external applications don't have. But an application using the ra API might have the same problem with ra_serf as svnrdump has. IMHO, the option to make ra_serf respect a more stricter interpretation of the editor api is something we need to give to a developer, not to a user via config. -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Enterprise Cloud Development Lieven
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
On Tue, Dec 11, 2012 at 9:29 PM, C. Michael Pilato cmpil...@collab.net wrote: On 12/10/2012 10:53 AM, C. Michael Pilato wrote: What if we revved the svn_ra_replay_range() API in such a way that it could now handle this initial revision scenario? We might add an 'incremental' flag that parallels what 'svnadmin dump' and 'svnrdump dump' do. This would get us to a single RA API used for revision replays (replay_range, instead of a combination of do_update + replay_range), which simplifies how replays are done from an API perspective. Of course, under the hood, the RA implementations of the new replay_range2() would probably just use the same update mechanics to handle that initial revision when not in incremental mode, but ra_serf's particular implementation would be free to choose send-all mode in this one scenario to do exactly that. Well, this turns out to be a little more sticky than I'd hoped. It's easy enough to add a send_complete_start_revision flag to svn_ra_replay_range() which causes the first transmitted revision to be a full dump of the tree as of that revision. But the svn_ra_replay_range() API also allows folks to specific whether they want *real* content change information, or just placeholder notifications for modified file content and node properties. Seems kinda yucky to transmit a full tree snapshot when the caller has asked not to get any real content mods; and we don't have a readily available way to send a full tree snapshot sans real content. Those technical challenges aside, I've since started to doubt the wisdom of adding special treatment of the starting revision to this API anyway. I'll continue pondering other options. What about my earlier suggestion? On Thu, Dec 6, 2012 at 11:04 PM, Mark Phippard markp...@gmail.com wrote: Is there an easy way to make svnrdump always use sendall mode? That would remove the release blocker we have about this command not working correctly with Serf and it seems like something we ought to be able to implement independent of these other questions. A bit of a hack, but after the config is read from file/registry and before svnrdump creates the ra_session we can set the global or per server-group bulk-updates option in memory to true. Is this for dump only? AFAIK, it is only dump where the svnrdump tool fails when using Serf. Because of the Ev1 stuff. Using bulk-updates ought to avoid that issue. Actually, my proposed hack of forcing ra_serf to use bulk update mode is not going to work. If the server has SVNAllowBulkUpdates set to Off, bulk update mode isn't available, the server will ignore any requests from the client and force skelta mode. If I remember correctly the issue here was responses arriving out of order due to multiple parallel connections. So the fix until svnrdump has moved to Ev2 is to stick to one extra connection for all the file content requests, so all of those responses arrive in sync. Don't know how easy that is to do, I wonder if we have a simple mechanism that allows a client to pass ra implementation specific options. It'd still require an ra API change, like guarantee_depth_first_order(), which ra_serf will use to limit the nr. of connections to 1, and probably only svnrdump will every use. -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Enterprise Cloud Development Lieven
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
On 12/12/2012 03:02 PM, Lieven Govaerts wrote:
On Tue, Dec 11, 2012 at 9:29 PM, C. Michael Pilato cmpil...@collab.net
wrote:
Those technical challenges aside, I've since started to doubt the wisdom of
adding special treatment of the starting revision to this API anyway. I'll
continue pondering other options.
What about my earlier suggestion?
I considered it.
And ... then I considered it a nasty hack. Seriously, this is really not
the kind of thing that *should* be exposed through an API.
...
svn_boolean_t honor_editor_api_promises;
...
Really? :-)
It does occur to me that one way to work around this is to add an API that
seems generally useful:
svn_ra_do_checkout()
This would be Yet Another Flavor Of Update-ish Thing, but wouldn't generate
a reporter/reporter_baton pair, and would immediately begin driving the
provided editor/editor_baton. And ra_serf's implementation thereof would,
of course, use send-all mode.
svnrdump is only trying to do essentally that anyway -- a update of
${NOTHING} to ${SOME_REV}. It calls svn_ra_do_update(), uses the provided
reporter to say I've got nothing, then finalizes the report and away she
goes. Would it not be more straightforward to offer a compact API for just
those sorts of use-cases -- the fake update from nothing to something
use-cases?
--
C. Michael Pilato cmpil...@collab.net
CollabNet www.collab.net Enterprise Cloud Development
signature.asc
Description: OpenPGP digital signature
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
On Wed, Dec 12, 2012 at 9:30 PM, C. Michael Pilato cmpil...@collab.net wrote:
On 12/12/2012 03:02 PM, Lieven Govaerts wrote:
On Tue, Dec 11, 2012 at 9:29 PM, C. Michael Pilato cmpil...@collab.net
wrote:
Those technical challenges aside, I've since started to doubt the wisdom of
adding special treatment of the starting revision to this API anyway. I'll
continue pondering other options.
What about my earlier suggestion?
I considered it.
And ... then I considered it a nasty hack. Seriously, this is really not
the kind of thing that *should* be exposed through an API.
...
svn_boolean_t honor_editor_api_promises;
...
Really? :-)
Depends a bit on how you name that function of course, you could call
it relax_editor_depth_first_promise(svn_boolean_t) - with default to
true. This is my reading of the conclusion of your issue 2932: we're
relaxing the rules a bit, but we think it's safe to do so we make this
the new default.
It does occur to me that one way to work around this is to add an API that
seems generally useful:
svn_ra_do_checkout()
This would be Yet Another Flavor Of Update-ish Thing, but wouldn't generate
a reporter/reporter_baton pair, and would immediately begin driving the
provided editor/editor_baton. And ra_serf's implementation thereof would,
of course, use send-all mode.
Note that send-all is not the solution if the server insists on skelta
mode (see my previous comment), so if you do an update or a checkout,
you still have to tell serf to limit to one connection.
svnrdump is only trying to do essentally that anyway -- a update of
${NOTHING} to ${SOME_REV}. It calls svn_ra_do_update(), uses the provided
reporter to say I've got nothing, then finalizes the report and away she
goes. Would it not be more straightforward to offer a compact API for just
those sorts of use-cases -- the fake update from nothing to something
use-cases?
Maybe, but would anything else besides svnrdump use this new API? svn
checkout/export solve this in a different manner already.
Lieven
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
On 12/12/2012 03:30 PM, C. Michael Pilato wrote:
On 12/12/2012 03:02 PM, Lieven Govaerts wrote:
On Tue, Dec 11, 2012 at 9:29 PM, C. Michael Pilato cmpil...@collab.net
wrote:
Those technical challenges aside, I've since started to doubt the wisdom of
adding special treatment of the starting revision to this API anyway. I'll
continue pondering other options.
What about my earlier suggestion?
I considered it.
And ... then I considered it a nasty hack.
Wait a second, though. If we're talking about nasty hacks, what about this
one: 'svnrdump dump' grows code to set the new bulk-updates config option on
behalf of the user. :-)
Something like this:
Index: subversion/svnrdump/svnrdump.c
===
--- subversion/svnrdump/svnrdump.c (revision 1420957)
+++ subversion/svnrdump/svnrdump.c (working copy)
@@ -341,7 +341,7 @@
apr_pool_t *pool)
{
svn_client_ctx_t *ctx = NULL;
- svn_config_t *cfg_config;
+ svn_config_t *cfg_config, *cfg_servers;
SVN_ERR(svn_ra_initialize(pool));
@@ -357,6 +357,13 @@
cfg_config = apr_hash_get(ctx-config, SVN_CONFIG_CATEGORY_CONFIG,
APR_HASH_KEY_STRING);
+ /* Forcibly prefer bulk-updates to work around issue #4116
+ (http://subversion.tigris.org/issues/show_bug.cgi?id=4116). */
+ cfg_servers = apr_hash_get(ctx-config, SVN_CONFIG_CATEGORY_SERVERS,
+ APR_HASH_KEY_STRING);
+ svn_config_set_bool(cfg_servers, SVN_CONFIG_SECTION_GLOBAL,
+ SVN_CONFIG_OPTION_BULK_UPDATES, TRUE);
+
/* Set up our cancellation support. */
ctx-cancel_func = check_cancel;
--
C. Michael Pilato cmpil...@collab.net
CollabNet www.collab.net Enterprise Cloud Development
signature.asc
Description: OpenPGP digital signature
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
Mike,
On Wed, Dec 12, 2012 at 10:15 PM, C. Michael Pilato cmpil...@collab.net wrote:
On 12/12/2012 03:30 PM, C. Michael Pilato wrote:
On 12/12/2012 03:02 PM, Lieven Govaerts wrote:
On Tue, Dec 11, 2012 at 9:29 PM, C. Michael Pilato cmpil...@collab.net
wrote:
Those technical challenges aside, I've since started to doubt the wisdom of
adding special treatment of the starting revision to this API anyway. I'll
continue pondering other options.
What about my earlier suggestion?
I considered it.
And ... then I considered it a nasty hack.
Wait a second, though. If we're talking about nasty hacks, what about this
one: 'svnrdump dump' grows code to set the new bulk-updates config option on
behalf of the user. :-)
You're not reading my mails at all are you? ;)
That's what I suggested before, but I found later that it will not
work because the server can force the client to use skelta mode
(SVNAllowBulkUpdates Off).
Something like this:
Index: subversion/svnrdump/svnrdump.c
===
--- subversion/svnrdump/svnrdump.c (revision 1420957)
+++ subversion/svnrdump/svnrdump.c (working copy)
@@ -341,7 +341,7 @@
apr_pool_t *pool)
{
svn_client_ctx_t *ctx = NULL;
- svn_config_t *cfg_config;
+ svn_config_t *cfg_config, *cfg_servers;
SVN_ERR(svn_ra_initialize(pool));
@@ -357,6 +357,13 @@
cfg_config = apr_hash_get(ctx-config, SVN_CONFIG_CATEGORY_CONFIG,
APR_HASH_KEY_STRING);
+ /* Forcibly prefer bulk-updates to work around issue #4116
+ (http://subversion.tigris.org/issues/show_bug.cgi?id=4116). */
+ cfg_servers = apr_hash_get(ctx-config, SVN_CONFIG_CATEGORY_SERVERS,
+ APR_HASH_KEY_STRING);
+ svn_config_set_bool(cfg_servers, SVN_CONFIG_SECTION_GLOBAL,
+ SVN_CONFIG_OPTION_BULK_UPDATES, TRUE);
+
Also, this setting can be set per server group by the user so
overriding the above, but I got the idea! ;)
/* Set up our cancellation support. */
ctx-cancel_func = check_cancel;
--
C. Michael Pilato cmpil...@collab.net
CollabNet www.collab.net Enterprise Cloud Development
Lieven
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
On 12/12/2012 04:04 PM, Lieven Govaerts wrote:
Note that send-all is not the solution if the server insists on skelta
mode (see my previous comment), so if you do an update or a checkout,
you still have to tell serf to limit to one connection.
And that's trickier than it seems, IIRC, because Ivan changed ra_serf to
recycle even the first connection as an auxiliary one once the REPORT is
completely processed.
svnrdump is only trying to do essentally that anyway -- a update of
${NOTHING} to ${SOME_REV}. It calls svn_ra_do_update(), uses the provided
reporter to say I've got nothing, then finalizes the report and away she
goes. Would it not be more straightforward to offer a compact API for just
those sorts of use-cases -- the fake update from nothing to something
use-cases?
Maybe, but would anything else besides svnrdump use this new API? svn
checkout/export solve this in a different manner already.
Well, in retrospect, I would name it something that did *not* have
checkout in the name: svn_ra_do_export(), or svn_ra_dump_tree(), or
something. And actually, I suspect that 'svn export' could use exactly this
function! ('svn checkout' could not, because it really *is* an update under
the hood.) But of course, it wouldn't do so because it prefers the more
performant async approach. All of which leads us back to turning away from
the send-all/non-send-all focus, and onto, as you said, limiting ourselves
to a single auxiliary fetch connection.
Still, we have to have a way to sensibly decide when to do so.
Here's a question I've been wondering for some time: should we expose to
users a configuration option for declaring the number of aux connections
ra_serf should use? I mean, Firefox has exposed such an option for years.
If we did so for Subversion, we could say, Yeah, ra_serf+svnrdump is a bad
combination. Here's a workaround. Run it with
--config-option=servers:global:serf-max-connections=2. Or better yet (as
per my other mail in this thread), we could just hardcode that option
override in svnrdump itself.
--
C. Michael Pilato cmpil...@collab.net
CollabNet www.collab.net Enterprise Cloud Development
signature.asc
Description: OpenPGP digital signature
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
On 12/12/2012 04:23 PM, Lieven Govaerts wrote: You're not reading my mails at all are you? ;) That's what I suggested before, but I found later that it will not work because the server can force the client to use skelta mode (SVNAllowBulkUpdates Off). I'm starting to think I must have missed some, for sure. Sorry about that. I'll re-read the whole thread (from archives) before continuing to blab on. -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Enterprise Cloud Development signature.asc Description: OpenPGP digital signature
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
On 12/10/2012 10:53 AM, C. Michael Pilato wrote: What if we revved the svn_ra_replay_range() API in such a way that it could now handle this initial revision scenario? We might add an 'incremental' flag that parallels what 'svnadmin dump' and 'svnrdump dump' do. This would get us to a single RA API used for revision replays (replay_range, instead of a combination of do_update + replay_range), which simplifies how replays are done from an API perspective. Of course, under the hood, the RA implementations of the new replay_range2() would probably just use the same update mechanics to handle that initial revision when not in incremental mode, but ra_serf's particular implementation would be free to choose send-all mode in this one scenario to do exactly that. Well, this turns out to be a little more sticky than I'd hoped. It's easy enough to add a send_complete_start_revision flag to svn_ra_replay_range() which causes the first transmitted revision to be a full dump of the tree as of that revision. But the svn_ra_replay_range() API also allows folks to specific whether they want *real* content change information, or just placeholder notifications for modified file content and node properties. Seems kinda yucky to transmit a full tree snapshot when the caller has asked not to get any real content mods; and we don't have a readily available way to send a full tree snapshot sans real content. Those technical challenges aside, I've since started to doubt the wisdom of adding special treatment of the starting revision to this API anyway. I'll continue pondering other options. -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Enterprise Cloud Development signature.asc Description: OpenPGP digital signature
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
On 12/07/2012 02:39 AM, Lieven Govaerts wrote: AFAIK, it is only dump where the svnrdump tool fails when using Serf. Because of the Ev1 stuff. Using bulk-updates ought to avoid that issue. Actually, my proposed hack of forcing ra_serf to use bulk update mode is not going to work. If the server has SVNAllowBulkUpdates set to Off, bulk update mode isn't available, the server will ignore any requests from the client and force skelta mode. If I remember correctly the issue here was responses arriving out of order due to multiple parallel connections. So the fix until svnrdump has moved to Ev2 is to stick to one extra connection for all the file content requests, so all of those responses arrive in sync. Don't know how easy that is to do, I wonder if we have a simple mechanism that allows a client to pass ra implementation specific options. IIUC, the problem with svnrdump occurs only for the single revision at the start of a non-incremental dump. svnrdump runs svn_ra_do_update2() in such a way as to pretend that it's doing a checkout. After that one revision, it runs a loop around svn_ra_replay_range(). Perhaps there's an opportunity here to solve two problems at once. What if we revved the svn_ra_replay_range() API in such a way that it could now handle this initial revision scenario? We might add an 'incremental' flag that parallels what 'svnadmin dump' and 'svnrdump dump' do. This would get us to a single RA API used for revision replays (replay_range, instead of a combination of do_update + replay_range), which simplifies how replays are done from an API perspective. Of course, under the hood, the RA implementations of the new replay_range2() would probably just use the same update mechanics to handle that initial revision when not in incremental mode, but ra_serf's particular implementation would be free to choose send-all mode in this one scenario to do exactly that. -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Enterprise Cloud Development signature.asc Description: OpenPGP digital signature
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
On Wed, Dec 5, 2012 at 6:18 PM, Lieven Govaerts l...@apache.org wrote: On Wed, Dec 5, 2012 at 9:54 PM, Mark Phippard markp...@gmail.com wrote: On Wed, Dec 5, 2012 at 3:46 PM, l...@apache.org wrote: Author: lgo Date: Wed Dec 5 20:46:33 2012 New Revision: 1417639 URL: http://svn.apache.org/viewvc?rev=1417639view=rev Log: Add a Force option to SVNAllowBulkUpdates. This allows a server admin to always respond to an update-report request with all content and properties inline. Now that skelta mode will be the new default with ra_serf, this feature can be useful in certain situations where the admin wants to avoid the overhead of per-file GET requests (e.g. with per-request Kerberos authentication). I appreciate that you are doing *something* here, but I still wonder if this is the right thing. It seems like it would be better if a 1.8 client tried to negotiate for the right to use skelta mode and it required a specifically configured 1.8 server to allow it. That's not how mod_dav_svn works now, the client negotiates the right to use bulk_update mode, which the server can accept (default) or deny by using the SVNAllowBulkUpdates flag. Given this flag already exists, it seems most logical to try and reuse it instead of adding a new option with the inverse meaning. Given that this can be implemented as new code, I do not think it is relevant how it works pre-1.8. For example, for things like merge tracking the server responds to the OPTIONS request with info that lets the client know it can use those methods. Why couldn't we do the same for skelta updates? (ASIDE: I am never sure how to describe the two possible modes) If a 1.8 serf client looked at the OPTIONS response to know if it can send this kind of request then old servers would never include this info and therefore Serf would only send Neon-style requests to those servers. IMO, this is the right behavior we should adopt. A 1.8 server could be configured to include this in the OPTIONS request. We can debate whether a 1.8 server should do this by default, with a configuration option to disable it, or whether it should not do this by default with an option to enable it. I think it should be the latter so that no one is surprised by the change. I see two problems with your approach: 1) Users running older servers, who are perhaps the most vulnerable to the new behavior, will not get any relief when 1.8 clients become the norm. I had no intention to change the default behavior of serf with this patch, in fact, that is a choice orthogonal to what I want to achieve here. Understood. I was just expressing what I see as the problems. Again, thanks for taking this on. Is there an easy way to make svnrdump always use sendall mode? That would remove the release blocker we have about this command not working correctly with Serf and it seems like something we ought to be able to implement independent of these other questions. -- Thanks Mark Phippard http://markphip.blogspot.com/
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
Hi, On Thu, Dec 6, 2012 at 3:44 PM, Mark Phippard markp...@gmail.com wrote: On Wed, Dec 5, 2012 at 6:18 PM, Lieven Govaerts l...@apache.org wrote: On Wed, Dec 5, 2012 at 9:54 PM, Mark Phippard markp...@gmail.com wrote: On Wed, Dec 5, 2012 at 3:46 PM, l...@apache.org wrote: Author: lgo Date: Wed Dec 5 20:46:33 2012 New Revision: 1417639 URL: http://svn.apache.org/viewvc?rev=1417639view=rev Log: Add a Force option to SVNAllowBulkUpdates. This allows a server admin to always respond to an update-report request with all content and properties inline. Now that skelta mode will be the new default with ra_serf, this feature can be useful in certain situations where the admin wants to avoid the overhead of per-file GET requests (e.g. with per-request Kerberos authentication). I appreciate that you are doing *something* here, but I still wonder if this is the right thing. It seems like it would be better if a 1.8 client tried to negotiate for the right to use skelta mode and it required a specifically configured 1.8 server to allow it. That's not how mod_dav_svn works now, the client negotiates the right to use bulk_update mode, which the server can accept (default) or deny by using the SVNAllowBulkUpdates flag. Given this flag already exists, it seems most logical to try and reuse it instead of adding a new option with the inverse meaning. Given that this can be implemented as new code, I do not think it is relevant how it works pre-1.8. For example, for things like merge tracking the server responds to the OPTIONS request with info that lets the client know it can use those methods. Why couldn't we do the same for skelta updates? (ASIDE: I am never sure how to describe the two possible modes) It is relevant because we already have a SVNAllowBulkUpdates flag that gives the admin some options to define this exact behavior. That flag gives the admin the option to disable bulk updates, to ensure that each individually request gets logged in access.log. Since it's perfectly possible to reuse the same flag with both options keeping their same meaning, I don't want to change that. If a 1.8 serf client looked at the OPTIONS response to know if it can send this kind of request then old servers would never include this info and therefore Serf would only send Neon-style requests to those servers. IMO, this is the right behavior we should adopt. A 1.8 server could be configured to include this in the OPTIONS request. We can debate whether a 1.8 server should do this by default, with a configuration option to disable it, or whether it should not do this by default with an option to enable it. I think it should be the latter so that no one is surprised by the change. With r1418071 and r1418077 the mechanism is now as follows: - with a 1.8 server, the admin can express how (s)he wants the clients to send update requests: SVNAllowBulkUpdates Off: only skelta mode allowed (as-is) SVNAllowBulkUpdates On: the client decides what mode to use (as-is) SVNAllowBulkUpdates Prefer: a well-behaved client should use bulk update mode. - a 1.8 ra_serf client will read the OPTIONS header representing the above 3 options. if the 1.8 server wants bulk mode Off or Prefer this is respected directly. (*) if the 1.8 server allows bulk mode but doesn't prefer it, ra_serf will use skelta mode. (*) If an older server doesn't send the header, ra_serf will use skelta mode (still the default) - the latter two options marked with (*) can be configured by the user with the bulk-updates option. I think this implements all options that are needed, and what you are suggesting except for the default mode to older servers. I see two problems with your approach: 1) Users running older servers, who are perhaps the most vulnerable to the new behavior, will not get any relief when 1.8 clients become the norm. I had no intention to change the default behavior of serf with this patch, in fact, that is a choice orthogonal to what I want to achieve here. Understood. I was just expressing what I see as the problems. Again, thanks for taking this on. Is there an easy way to make svnrdump always use sendall mode? That would remove the release blocker we have about this command not working correctly with Serf and it seems like something we ought to be able to implement independent of these other questions. A bit of a hack, but after the config is read from file/registry and before svnrdump creates the ra_session we can set the global or per server-group bulk-updates option in memory to true. Is this for dump only? -- Thanks Mark Phippard http://markphip.blogspot.com/ Lieven
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
On Thu, Dec 6, 2012 at 4:40 PM, Lieven Govaerts l...@apache.org wrote: It is relevant because we already have a SVNAllowBulkUpdates flag that gives the admin some options to define this exact behavior. That flag gives the admin the option to disable bulk updates, to ensure that each individually request gets logged in access.log. Since it's perfectly possible to reuse the same flag with both options keeping their same meaning, I don't want to change that. My main concern with reusing this flag is that I did not want to force us down a path where we are recommending users setup their server with SVNAllowBulkUpdates off because this option will impact all users running pre-1.8 clients (Neon). Assuming prefer does not impact Neon, it sounds like there would be little reason for someone to use off unless they really wanted that behavior for all clients. With r1418071 and r1418077 the mechanism is now as follows: - with a 1.8 server, the admin can express how (s)he wants the clients to send update requests: SVNAllowBulkUpdates Off: only skelta mode allowed (as-is) SVNAllowBulkUpdates On: the client decides what mode to use (as-is) SVNAllowBulkUpdates Prefer: a well-behaved client should use bulk update mode. - a 1.8 ra_serf client will read the OPTIONS header representing the above 3 options. if the 1.8 server wants bulk mode Off or Prefer this is respected directly. What will a Neon client do with Prefer? Just ignore it and treat it like On? (*) if the 1.8 server allows bulk mode but doesn't prefer it, ra_serf will use skelta mode. (*) If an older server doesn't send the header, ra_serf will use skelta mode (still the default) - the latter two options marked with (*) can be configured by the user with the bulk-updates option. I think this implements all options that are needed, and what you are suggesting except for the default mode to older servers. Overall it is good, but I guess I just happen to think it does not cover the case that I thought was most important (older servers). It does at least give those users an incentive to upgrade since they can then control the behavior if they need to. I guess we would have to tell these users to beg their users to edit their local configs in the meantime? Is there an easy way to make svnrdump always use sendall mode? That would remove the release blocker we have about this command not working correctly with Serf and it seems like something we ought to be able to implement independent of these other questions. A bit of a hack, but after the config is read from file/registry and before svnrdump creates the ra_session we can set the global or per server-group bulk-updates option in memory to true. Is this for dump only? AFAIK, it is only dump where the svnrdump tool fails when using Serf. Because of the Ev1 stuff. Using bulk-updates ought to avoid that issue. I wonder if there is an XFail test currently? -- Thanks Mark Phippard http://markphip.blogspot.com/
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
On Wed, Dec 05, 2012 at 03:54:00PM -0500, Mark Phippard wrote: On Wed, Dec 5, 2012 at 3:46 PM, l...@apache.org wrote: Author: lgo Date: Wed Dec 5 20:46:33 2012 New Revision: 1417639 URL: http://svn.apache.org/viewvc?rev=1417639view=rev Log: Add a Force option to SVNAllowBulkUpdates. This allows a server admin to always respond to an update-report request with all content and properties inline. Now that skelta mode will be the new default with ra_serf, this feature can be useful in certain situations where the admin wants to avoid the overhead of per-file GET requests (e.g. with per-request Kerberos authentication). I appreciate that you are doing *something* here, but I still wonder if this is the right thing. It seems like it would be better if a 1.8 client tried to negotiate for the right to use skelta mode and it required a specifically configured 1.8 server to allow it. I see two problems with your approach: 1) Users running older servers, who are perhaps the most vulnerable to the new behavior, will not get any relief when 1.8 clients become the norm. 2) Wouldn't this new option break any users that happen to be using Serf with a pre-1.8 client? I think Serf clients should default to sendall mode unless the OPTIONS includes something that says it is OK to use skelta mode. This would force 1.8 clients to behave like Neon clients when talking to older servers and it would allow admins that upgrade their servers to 1.8 to get whatever benefits they see from enabling the skelta mode. Just my 2 cents Makes sense. We could also consider making 1.8 server advertise skelta mode capability by default, with an option to turn it off, so that 1.8 clients will negotiate skelta mode with 1.8 servers by default, but not with earlier servers. This way, 1.8 clients talking to 1.8 servers can take advantage of skelta mode, while clients talking to older servers won't stress out existing installations by default. In the 1.8 release notes We can raise attention in the fact that a 1.8 server should turn off skelta mode if necessary, such as when each GET request causes an authentication request to LDAP.
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
Hi, On Thu, Dec 6, 2012 at 11:04 PM, Mark Phippard markp...@gmail.com wrote: On Thu, Dec 6, 2012 at 4:40 PM, Lieven Govaerts l...@apache.org wrote: It is relevant because we already have a SVNAllowBulkUpdates flag that gives the admin some options to define this exact behavior. That flag gives the admin the option to disable bulk updates, to ensure that each individually request gets logged in access.log. Since it's perfectly possible to reuse the same flag with both options keeping their same meaning, I don't want to change that. My main concern with reusing this flag is that I did not want to force us down a path where we are recommending users setup their server with SVNAllowBulkUpdates off because this option will impact all users running pre-1.8 clients (Neon). Assuming prefer does not impact Neon, it sounds like there would be little reason for someone to use off unless they really wanted that behavior for all clients. If you want to advise users to profit from the skelta-mode in serf, than they should use the default On. If you want to advise users to stick to the old neon-style mode of bulk updates, they should set it to Prefer. Setting Off for this flag still has its old purpose, when its mandatory that all individual file accesses are logged, for new and old clients. With r1418071 and r1418077 the mechanism is now as follows: - with a 1.8 server, the admin can express how (s)he wants the clients to send update requests: SVNAllowBulkUpdates Off: only skelta mode allowed (as-is) SVNAllowBulkUpdates On: the client decides what mode to use (as-is) SVNAllowBulkUpdates Prefer: a well-behaved client should use bulk update mode. - a 1.8 ra_serf client will read the OPTIONS header representing the above 3 options. if the 1.8 server wants bulk mode Off or Prefer this is respected directly. What will a Neon client do with Prefer? Just ignore it and treat it like On? A pre-1.8 neon client will not see this new header, so it will send the send-all header in the update-report. The server treats this as I've told the client my preference, now do what the client asks me to, so the server will use the bulk update mode, just like before. A 1.8 neon client doesn't exist. (*) if the 1.8 server allows bulk mode but doesn't prefer it, ra_serf will use skelta mode. (*) If an older server doesn't send the header, ra_serf will use skelta mode (still the default) - the latter two options marked with (*) can be configured by the user with the bulk-updates option. I think this implements all options that are needed, and what you are suggesting except for the default mode to older servers. Overall it is good, but I guess I just happen to think it does not cover the case that I thought was most important (older servers). It does at least give those users an incentive to upgrade since they can then control the behavior if they need to. I guess we would have to tell these users to beg their users to edit their local configs in the meantime? It doesn't set the default of serf to bulk when talking with older servers, that's true. But the mechanism is there, should we decide that's the correct thing to do it's easily added. Is there an easy way to make svnrdump always use sendall mode? That would remove the release blocker we have about this command not working correctly with Serf and it seems like something we ought to be able to implement independent of these other questions. A bit of a hack, but after the config is read from file/registry and before svnrdump creates the ra_session we can set the global or per server-group bulk-updates option in memory to true. Is this for dump only? AFAIK, it is only dump where the svnrdump tool fails when using Serf. Because of the Ev1 stuff. Using bulk-updates ought to avoid that issue. Actually, my proposed hack of forcing ra_serf to use bulk update mode is not going to work. If the server has SVNAllowBulkUpdates set to Off, bulk update mode isn't available, the server will ignore any requests from the client and force skelta mode. If I remember correctly the issue here was responses arriving out of order due to multiple parallel connections. So the fix until svnrdump has moved to Ev2 is to stick to one extra connection for all the file content requests, so all of those responses arrive in sync. Don't know how easy that is to do, I wonder if we have a simple mechanism that allows a client to pass ra implementation specific options. I wonder if there is an XFail test currently? No: lgo-macbp:trunk lgo$ cat subversion/tests/cmdline/svnrdump_tests.py | grep XFAIL lgo-macbp:trunk lgo$ The parallel connections are only opened after a minimum number of requests, probably our svnrdump test repository aren't big enough. -- Thanks Mark Phippard http://markphip.blogspot.com/ Lieven
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
On Wed, Dec 5, 2012 at 10:50 PM, Stefan Sperling s...@elego.de wrote: On Wed, Dec 05, 2012 at 03:54:00PM -0500, Mark Phippard wrote: On Wed, Dec 5, 2012 at 3:46 PM, l...@apache.org wrote: Author: lgo Date: Wed Dec 5 20:46:33 2012 New Revision: 1417639 URL: http://svn.apache.org/viewvc?rev=1417639view=rev Log: Add a Force option to SVNAllowBulkUpdates. This allows a server admin to always respond to an update-report request with all content and properties inline. Now that skelta mode will be the new default with ra_serf, this feature can be useful in certain situations where the admin wants to avoid the overhead of per-file GET requests (e.g. with per-request Kerberos authentication). I appreciate that you are doing *something* here, but I still wonder if this is the right thing. It seems like it would be better if a 1.8 client tried to negotiate for the right to use skelta mode and it required a specifically configured 1.8 server to allow it. I see two problems with your approach: 1) Users running older servers, who are perhaps the most vulnerable to the new behavior, will not get any relief when 1.8 clients become the norm. 2) Wouldn't this new option break any users that happen to be using Serf with a pre-1.8 client? I think Serf clients should default to sendall mode unless the OPTIONS includes something that says it is OK to use skelta mode. This would force 1.8 clients to behave like Neon clients when talking to older servers and it would allow admins that upgrade their servers to 1.8 to get whatever benefits they see from enabling the skelta mode. Just my 2 cents Makes sense. We could also consider making 1.8 server advertise skelta mode capability by default, with an option to turn it off, That's de facto what happens, although the wording of the options is slightly different. A 1.8 server will now always send the SVN-Allow-Bulk-Updates header, with On|Off|Prefer as possible values. By default this value is set to On (default value of SVNAllowBulkUpdates), so the client is allowed to decide, which in case of a 1.8 client with ra_serf is skelta mode. so that 1.8 clients will negotiate skelta mode with 1.8 servers by default, but not with earlier servers. This way, 1.8 clients talking to 1.8 servers can take advantage of skelta mode, while clients talking to older servers won't stress out existing installations by default. As I already responded to Mark, I haven't changed the ra_serf defaults to this proposed behavior, but the option is now there. In the 1.8 release notes We can raise attention in the fact that a 1.8 server should turn off skelta mode if necessary, such as when each GET request causes an authentication request to LDAP. Yeah, we should explain what the new default behavior of a 1.8 client is, and in which situations the admin can alter it with SVNAllowBulkUpdates Prefer. Lieven
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
On Wed, Dec 5, 2012 at 4:50 PM, Stefan Sperling s...@elego.de wrote: In the 1.8 release notes We can raise attention in the fact that a 1.8 server should turn off skelta mode if necessary, such as when each GET request causes an authentication request to LDAP. +1. I'm all for documenting and giving knobs to users who RTFM. My very strong preference is that we should do the right thing out of the box - and that is to use parallel updates - even against older servers. This has always been the behavior of ra_serf and it's definitely backwards compatible with all 1.x servers. If an admin doesn't like it (because of per-req authn or bad logging subsystems or the phase of the moon), they can try to direct 1.8+ clients away from parallel updates via the new directive/headers; but, we should not require any configurations to do the best generically recommended approach. -- justin
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
On Wed, Dec 5, 2012 at 3:46 PM, l...@apache.org wrote: Author: lgo Date: Wed Dec 5 20:46:33 2012 New Revision: 1417639 URL: http://svn.apache.org/viewvc?rev=1417639view=rev Log: Add a Force option to SVNAllowBulkUpdates. This allows a server admin to always respond to an update-report request with all content and properties inline. Now that skelta mode will be the new default with ra_serf, this feature can be useful in certain situations where the admin wants to avoid the overhead of per-file GET requests (e.g. with per-request Kerberos authentication). I appreciate that you are doing *something* here, but I still wonder if this is the right thing. It seems like it would be better if a 1.8 client tried to negotiate for the right to use skelta mode and it required a specifically configured 1.8 server to allow it. I see two problems with your approach: 1) Users running older servers, who are perhaps the most vulnerable to the new behavior, will not get any relief when 1.8 clients become the norm. 2) Wouldn't this new option break any users that happen to be using Serf with a pre-1.8 client? I think Serf clients should default to sendall mode unless the OPTIONS includes something that says it is OK to use skelta mode. This would force 1.8 clients to behave like Neon clients when talking to older servers and it would allow admins that upgrade their servers to 1.8 to get whatever benefits they see from enabling the skelta mode. Just my 2 cents -- Thanks Mark Phippard http://markphip.blogspot.com/
Re: svn commit: r1417639 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c reports/update.c
On Wed, Dec 5, 2012 at 9:54 PM, Mark Phippard markp...@gmail.com wrote: On Wed, Dec 5, 2012 at 3:46 PM, l...@apache.org wrote: Author: lgo Date: Wed Dec 5 20:46:33 2012 New Revision: 1417639 URL: http://svn.apache.org/viewvc?rev=1417639view=rev Log: Add a Force option to SVNAllowBulkUpdates. This allows a server admin to always respond to an update-report request with all content and properties inline. Now that skelta mode will be the new default with ra_serf, this feature can be useful in certain situations where the admin wants to avoid the overhead of per-file GET requests (e.g. with per-request Kerberos authentication). I appreciate that you are doing *something* here, but I still wonder if this is the right thing. It seems like it would be better if a 1.8 client tried to negotiate for the right to use skelta mode and it required a specifically configured 1.8 server to allow it. That's not how mod_dav_svn works now, the client negotiates the right to use bulk_update mode, which the server can accept (default) or deny by using the SVNAllowBulkUpdates flag. Given this flag already exists, it seems most logical to try and reuse it instead of adding a new option with the inverse meaning. I see two problems with your approach: 1) Users running older servers, who are perhaps the most vulnerable to the new behavior, will not get any relief when 1.8 clients become the norm. I had no intention to change the default behavior of serf with this patch, in fact, that is a choice orthogonal to what I want to achieve here. The intention here is for server admins to decide if their setup is known to be not ready for parallel update. 2) Wouldn't this new option break any users that happen to be using Serf with a pre-1.8 client? Yes, indeed it will. I haven't really thought yet about whether or not that's a problem. If it is, then that means we have no possibility to give the admin the option to force the use of bulk updates. The closest alternative than is to make it a Prefer option, and use OPTIONS to ask the client to kindly respect the server's preference for bulk updates. Which a pre-1.8 client with serf will then ignore. I think Serf clients should default to sendall mode unless the OPTIONS includes something that says it is OK to use skelta mode. This would force 1.8 clients to behave like Neon clients when talking to older servers and it would allow admins that upgrade their servers to 1.8 to get whatever benefits they see from enabling the skelta mode. Again, this was not the purpose of this patch, but this can be added provided that's what the svn dev community decides is the best approach. Just my 2 cents Appreciated. -- Thanks Mark Phippard http://markphip.blogspot.com/ Lieven
