{"thread":{"id":"34360","subject":"[PATCH 0/2] allow git-svn fetching to work using serf","startedAt":"2013-07-06T03:41:58Z","lastAt":"2013-07-07T17:53:16Z","messageCount":9,"participants":["Kyle McKay","David Rothenberger","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"222662","messageId":"CB53C901-3643-46AE-AA80-CED5E20AC3B7@gmail.com","threadId":"34360","inReplyTo":null,"subject":"[PATCH 0/2] allow git-svn fetching to work using serf","fromName":"Kyle McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-06T03:41:58Z","receivedAt":"2013-07-06T03:41:58Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"This patch allows git-svn to fetch successfully using the\nserf library when given an https?: url to fetch from.\n\nUnfortunately some svn servers do not seem to be configured\nwell for use with the serf library.  This can cause fetching\nto take longer compared to the neon library or actually\ncause timeouts during the fetch.  When timeouts occur\ngit-svn can be safely restarted to fetch more revisions.\n\nA new temp_is_locked function has been added to Git.pm\nto facilitate using the minimal number of temp files\npossible when using serf.\n\nThe problem that occurs when running git-svn fetch using\nthe serf library is that the previously used temp file\nis not always unlocked before the next temp file needs\nto be used.\n\nTo work around this problem, a new temp name is used\nif the temp name that would otherwise be chosen is\ncurrently locked.\n\nThis patch may not be all that is required, but at least\nit is a starting point.\n\nDaniel Shahaf has suggested also setting \"servers:global:http-bulk- \nupdates=on\".\n\nKyle J. McKay (2):\n  Git.pm: add new temp_is_locked function\n  git-svn: allow git-svn fetching to work using serf\n\nperl/Git.pm             | 17 ++++++++++++++++-\nperl/Git/SVN/Fetcher.pm |  6 ++++--\n2 files changed, 20 insertions(+), 3 deletions(-)\n\n-- \n1.8.3\n"},{"id":"222667","messageId":"51D7C47D.5070700@acm.org","threadId":"34360","inReplyTo":"CB53C901-3643-46AE-AA80-CED5E20AC3B7@gmail.com","subject":"Re: [PATCH 0/2] allow git-svn fetching to work using serf","fromName":"David Rothenberger","fromEmail":"daveroth@acm.org","sentAt":"2013-07-06T07:17:17Z","receivedAt":"2013-07-06T07:17:17Z","isPatch":true,"sender":{"key":"daveroth@acm.org","avatar":null},"body":"On 7/5/2013 8:41 PM, Kyle McKay wrote:\n> This patch allows git-svn to fetch successfully using the\n> serf library when given an https?: url to fetch from.\n\nThanks, Kyle. I confirm this is working for my problem cases as\nwell.\n\n> Daniel Shahaf has suggested also setting\n> \"servers:global:http-bulk-updates=on\".\n\nI have a patch that does this, but since turning on bulk updates has\na possible performance penalty, I prefer your approach. \n\nPlease let me know if anyone wants to see the patch.\n\n-- \nDavid Rothenberger  ----  daveroth@acm.org\n\nWhat to do in case of an alien attack:\n\n    1)   Hide beneath the seat of your plane and look away.\n    2)   Avoid eye contact.\n    3) If there are no eyes, avoid all contact.\n\n                -- The Firesign Theatre, _Everything you know is Wrong_\n"},{"id":"222679","messageId":"20130707002804.GF30132@google.com","threadId":"34360","inReplyTo":"51D7C47D.5070700@acm.org","subject":"Re: [PATCH 0/2] allow git-svn fetching to work using serf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-07-07T00:28:04Z","receivedAt":"2013-07-07T00:28:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"David Rothenberger wrote:\n> On 7/5/2013 8:41 PM, Kyle McKay wrote:\n\n>> Daniel Shahaf has suggested also setting\n>> \"servers:global:http-bulk-updates=on\".\n>\n> I have a patch that does this, but since turning on bulk updates has\n> a possible performance penalty, I prefer your approach. \n\nI assume that's because http-bulk-updates defeats caching.  If so,\nmakes sense.\n\nPlease forgive my ignorance: is there a bug filed about ra_serf's\nmisbehavior here?  Is it eventually going to be fixed and this is\njust a workaround, or is the growth in temp file use something we'd\nlive with permanently?\n\nThanks,\nJonathan\n"},{"id":"222684","messageId":"1D11122F-5C75-4FAC-80EA-D5DC65902403@gmail.com","threadId":"34360","inReplyTo":"20130707002804.GF30132@google.com","subject":"Re: [PATCH 0/2] allow git-svn fetching to work using serf","fromName":"Kyle McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-07T01:24:35Z","receivedAt":"2013-07-07T01:24:35Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Jul 6, 2013, at 17:28, Jonathan Nieder wrote:\n> David Rothenberger wrote:\n>> On 7/5/2013 8:41 PM, Kyle McKay wrote:\n>\n>>> Daniel Shahaf has suggested also setting\n>>> \"servers:global:http-bulk-updates=on\".\n>>\n>> I have a patch that does this, but since turning on bulk updates has\n>> a possible performance penalty, I prefer your approach.\n>\n> I assume that's because http-bulk-updates defeats caching.  If so,\n> makes sense.\n>\n> Please forgive my ignorance: is there a bug filed about ra_serf's\n> misbehavior here?  Is it eventually going to be fixed and this is\n> just a workaround, or is the growth in temp file use something we'd\n> live with permanently?\n\nApparently it will not be fixed:\n\nBegin forwarded message:\n> From: David Rothenberger <daveroth@acm.org>\n> Date: July 5, 2013 16:14:12 PDT\n> To: git@vger.kernel.org\n> Subject: Re: git-svn \"Temp file with moniker 'svn_delta' already in  \n> use\" and skelta mode\n>\n> I traced git-svn and discovered that the error is due to a known\n> problem in the SVN APIs. ra_serf does not drive the delta editor in\n> a depth-first manner as required by the API [1]. Instead, the calls\n> come in this order:\n>\n> 1. open_root\n> 2. open_directory\n> 3. add_file\n> 4. apply_textdelta\n> 5. add_file\n> 6. apply_textdelta\n>\n> This is a known issue [2] and one that the Subversion folks have\n> elected not to fix [3].\n>\n> [1]\n> http://subversion.apache.org/docs/api/latest/structsvn__delta__editor__t.html#details\n> [2] http://subversion.tigris.org/issues/show_bug.cgi?id=2932\n> [3] http://subversion.tigris.org/issues/show_bug.cgi?id=3831\n\nThe summary of [3] which is marked RESOLVED,FIXED is \"Add errata / \nrelease note noise around ra_serf's editor drive violations\".\n\nKyle\n"},{"id":"222685","messageId":"20130707013747.GM30132@google.com","threadId":"34360","inReplyTo":"1D11122F-5C75-4FAC-80EA-D5DC65902403@gmail.com","subject":"Re: [PATCH 0/2] allow git-svn fetching to work using serf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-07-07T01:37:47Z","receivedAt":"2013-07-07T01:37:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Kyle McKay wrote:\n> On Jul 6, 2013, at 17:28, Jonathan Nieder wrote:\n>> David Rothenberger wrote:\n>>> On 7/5/2013 8:41 PM, Kyle McKay wrote:\n\n>>>> Daniel Shahaf has suggested also setting\n>>>> \"servers:global:http-bulk-updates=on\".\n>>>\n>>> I have a patch that does this, but since turning on bulk updates has\n>>> a possible performance penalty, I prefer your approach.\n>>\n>> I assume that's because http-bulk-updates defeats caching.  If so,\n>> makes sense.\n>>\n>> Please forgive my ignorance: is there a bug filed about ra_serf's\n>> misbehavior here?  Is it eventually going to be fixed and this is\n>> just a workaround, or is the growth in temp file use something we'd\n>> live with permanently?\n[...]\n>\n> Begin forwarded message:\n[...]\n>> [2] http://subversion.tigris.org/issues/show_bug.cgi?id=2932\n\nAh, thanks for the context.\n\nIt's still not clear to me how we know that ra_serf driving the editor\nin a non depth-first manner is the problem here.  Has that explanation\nbeen confirmed somehow?\n\nFor example, does the workaround mentioned by danielsh work?  Does\nusing ra_neon instead of ra_serf avoid trouble as well?  Is there a\nsimple explanation of why violating the depth-first constraint would\nlead to multiple blob (i.e., file, not directory) deltas being opened\nin a row without an intervening close?\n\nJonathan\n"},{"id":"222689","messageId":"FBCA37F9-4988-4773-8D8D-9CB041C35289@gmail.com","threadId":"34360","inReplyTo":"20130707013747.GM30132@google.com","subject":"Re: [PATCH 0/2] allow git-svn fetching to work using serf","fromName":"Kyle McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-07T02:46:38Z","receivedAt":"2013-07-07T02:46:38Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Jul 6, 2013, at 18:37, Jonathan Nieder wrote:\n> Kyle McKay wrote:\n>> On Jul 6, 2013, at 17:28, Jonathan Nieder wrote:\n>>> David Rothenberger wrote:\n>>>> On 7/5/2013 8:41 PM, Kyle McKay wrote:\n>\n>>>>> Daniel Shahaf has suggested also setting\n>>>>> \"servers:global:http-bulk-updates=on\".\n>>>>\n>>>> I have a patch that does this, but since turning on bulk updates  \n>>>> has\n>>>> a possible performance penalty, I prefer your approach.\n>>>\n>>> I assume that's because http-bulk-updates defeats caching.  If so,\n>>> makes sense.\n>>>\n>>> Please forgive my ignorance: is there a bug filed about ra_serf's\n>>> misbehavior here?  Is it eventually going to be fixed and this is\n>>> just a workaround, or is the growth in temp file use something we'd\n>>> live with permanently?\n> [...]\n>>\n>> Begin forwarded message:\n> [...]\n>>> [2] http://subversion.tigris.org/issues/show_bug.cgi?id=2932\n>\n> Ah, thanks for the context.\n>\n> It's still not clear to me how we know that ra_serf driving the editor\n> in a non depth-first manner is the problem here.  Has that explanation\n> been confirmed somehow?\n>\n> For example, does the workaround mentioned by danielsh work?  Does\n> using ra_neon instead of ra_serf avoid trouble as well?  Is there a\n> simple explanation of why violating the depth-first constraint would\n> lead to multiple blob (i.e., file, not directory) deltas being opened\n> in a row without an intervening close?\n\nUsing ra_neon seams to eliminate the problem. Using ra_neon has always  \nbeen the default until svn 1.8 which drops ra_neon support entirely  \nand always uses ra_serf for https?: urls.\n\nThe workaround mentioned by danielsh won't work if the server has  \nconfigured SVNAllowBulkUpdates Off because that will force use of  \nskelta mode no matter what the client does.  However, since ra_neon  \nonly ever has a single connection to the server it probably doesn't  \nmatter.\n\nSince ra_serf makes multiple connections to the server (hard-coded to  \n4 prior to svn 1.8, defaults to 4 in svn 1.8 but can be set to between  \n1 and 8) it makes sense there would be multiple active calls to  \napply_textdelta if processing is done as results are received on the  \nmultiple connections.\n\nKyle\n"},{"id":"222694","messageId":"51D8E40F.2020008@acm.org","threadId":"34360","inReplyTo":"20130707002804.GF30132@google.com","subject":"Re: [PATCH 0/2] allow git-svn fetching to work using serf","fromName":"David Rothenberger","fromEmail":"daveroth@acm.org","sentAt":"2013-07-07T03:44:15Z","receivedAt":"2013-07-07T03:44:15Z","isPatch":true,"sender":{"key":"daveroth@acm.org","avatar":null},"body":"On 7/6/2013 5:28 PM, Jonathan Nieder wrote:\n> David Rothenberger wrote:\n>> On 7/5/2013 8:41 PM, Kyle McKay wrote:\n> \n>>> Daniel Shahaf has suggested also setting\n>>> \"servers:global:http-bulk-updates=on\".\n>>\n>> I have a patch that does this, but since turning on bulk updates has\n>> a possible performance penalty, I prefer your approach. \n> \n> I assume that's because http-bulk-updates defeats caching.  If so,\n> makes sense.\n\nI believe that \"bulk updates\" means that serf makes one request for a\nlot of information and receives it all in one big response. In \"skelta\"\nmode, serf makes a single request for a single piece of information. The\nserf authors feel this can lead to improved overall throughput because\nthey can pipeline these requests and have multiple connections open at\nthe same time.\n\nThe downside, though, is that serf will do multiple open_file calls in\nparallel as it descends down sibling directories.\n\n> It's still not clear to me how we know that ra_serf driving the editor\n> in a non depth-first manner is the problem here.  Has that explanation\n> been confirmed somehow?\n\nI did do a trace of \"git svn fetch\" and observed this non-depth-first\ntraversal. It certainly causes the failure we've observed.\n\n> Is there a simple explanation of why violating the depth-first\n> constraint would lead to multiple blob (i.e., file, not directory)\n> deltas being opened in a row without an intervening close?\n\nI believe serf is doing the following for a number of files in parallel:\n 1. open_file\n 2. apply_textdelta\n 3. change_file_prop, change_file_prop, ...\n 4. close_file\n\n\n-- \nDavid Rothenberger  ----  daveroth@acm.org\n\nNusbaum's Rule:\n        The more pretentious the corporate name, the smaller the\n        organization.  (For instance, the Murphy Center for the\n        Codification of Human and Organizational Law, contrasted\n        to IBM, GM, and AT&T.)\n"},{"id":"222754","messageId":"20130707174043.GA9975@google.com","threadId":"34360","inReplyTo":"FBCA37F9-4988-4773-8D8D-9CB041C35289@gmail.com","subject":"Re: [PATCH 0/2] allow git-svn fetching to work using serf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-07-07T17:40:43Z","receivedAt":"2013-07-07T17:40:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(cc-ing subversion's users@ list for advice)\nKyle McKay wrote:\n> On Jul 6, 2013, at 18:37, Jonathan Nieder wrote:\n>> Kyle McKay wrote:\n>>> Begin forwarded message:\n\n>>>> [2] http://subversion.tigris.org/issues/show_bug.cgi?id=2932\n>>\n>> Ah, thanks for the context.\n>>\n>> It's still not clear to me how we know that ra_serf driving the editor\n>> in a non depth-first manner is the problem here.  Has that explanation\n>> been confirmed somehow?\n[...]\n> Since ra_serf makes multiple connections to the server (hard-coded\n> to 4 prior to svn 1.8, defaults to 4 in svn 1.8 but can be set to\n> between 1 and 8) it makes sense there would be multiple active calls\n> to apply_textdelta if processing is done as results are received on\n> the multiple connections.\n\nAh, that's worrisome.  Do I understand you correctly that to work with\nra_serf in skelta mode, callers need to make their apply_textdelta\ncallback thread-safe?\n\nOr do you just mean that the traversal order is based on the order in\nwhich results are received?  That would be fine, as long as after each\napply_textdelta call, close_file is called before the next\napply_textdelta.\n\nJonathan\n"},{"id":"222757","messageId":"20130707175316.GB9975@google.com","threadId":"34360","inReplyTo":"51D8E40F.2020008@acm.org","subject":"Re: [PATCH 0/2] allow git-svn fetching to work using serf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-07-07T17:53:16Z","receivedAt":"2013-07-07T17:53:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(cc-ing users@ as requested by danielsh)\nDavid Rothenberger wrote:\n> On 7/6/2013 5:28 PM, Jonathan Nieder wrote:\n\n>> Is there a simple explanation of why violating the depth-first\n>> constraint would lead to multiple blob (i.e., file, not directory)\n>> deltas being opened in a row without an intervening close?\n>\n> I believe serf is doing the following for a number of files in parallel:\n>  1. open_file\n>  2. apply_textdelta\n>  3. change_file_prop, change_file_prop, ...\n>  4. close_file\n\nAh, that makes more sense.  It is not about traversal order but about\nprocessing multiple non-directory files in parallel, and step (3)\npotentially involving a large number of property changes means that it\ncan make sense not to take a lock.\n\nPerhaps the reference documentation could warn about this?\n\nOn the git-svn side, it looks like we have enough information to make\na more complete commit message or in-code comment so the reason for\nmultiple git_blob tempfiles is not forgotten.  Thanks for your patient\nexplanations.\n\nJonathan\n"}]}