Return-Path: X-Original-To: apmail-subversion-dev-archive@minotaur.apache.org Delivered-To: apmail-subversion-dev-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id D54A01100A for ; Mon, 18 Aug 2014 14:35:29 +0000 (UTC) Received: (qmail 89930 invoked by uid 500); 18 Aug 2014 14:35:29 -0000 Delivered-To: apmail-subversion-dev-archive@subversion.apache.org Received: (qmail 89883 invoked by uid 500); 18 Aug 2014 14:35:29 -0000 Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Delivered-To: mailing list dev@subversion.apache.org Received: (qmail 89860 invoked by uid 99); 18 Aug 2014 14:35:29 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 18 Aug 2014 14:35:29 +0000 X-ASF-Spam-Status: No, hits=1.5 required=5.0 tests=HTML_MESSAGE,RCVD_IN_DNSWL_LOW,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (nike.apache.org: domain of stefan.fuhrmann@wandisco.com designates 209.85.213.175 as permitted sender) Received: from [209.85.213.175] (HELO mail-ig0-f175.google.com) (209.85.213.175) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 18 Aug 2014 14:35:03 +0000 Received: by mail-ig0-f175.google.com with SMTP id uq10so8306364igb.14 for ; Mon, 18 Aug 2014 07:35:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=wandisco.com; s=gapps; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=m/J9uf1h4WDHdh6f0kvn6KQ4rKI8cdWAJ9QXmWGP8j0=; b=NfWUKqeS+NoPHvfOyHtfN1Riuv2JpxqyA1HTB/E5Ca8XLVewWZ99D2TrcitpbLCPqC m6mDt7GjuE0sp/qJFAZojJobeHh2L6LpEg3smZdU3fOMPFKcLy4Zs9NgLUl58nYrW+EE IpC0a6gRdL6Zz6dGyUqR8HrYY/EZ5dXA0u6Fs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=m/J9uf1h4WDHdh6f0kvn6KQ4rKI8cdWAJ9QXmWGP8j0=; b=GU+Jto5kLGqaQB73SAslfXj6vByEhmnUcaF86MQ7pdy0Uzxb3QNLC+kG/AIRHEpvfB D91kprAtYNRIILysyXWcvbPdiywSELpMMy5UxBDNm/fo7hPC2FdxCgHEXllIWcc/Dm5w zxSprpuU/AFNL3C+7K5Mp+Ty9AXCDHfGT95MkxC6uIvfIYgJrkOPKOwqemSHTw8BAj5/ GnePBbIYa3iRH/sJzQEorOzOvM/3b8ciSauJSGQaJMM9V2WYxOV4hKKRLNsQKZxBbGqd bkP+6WGBj6aoniTtrKBiQT3LQiFyLphsp9bvEGFM6eGU+6JDrBU8YD2RHdalmGVGju0r tmcQ== X-Gm-Message-State: ALoCoQkf3DoABd7zGV+uzAH08WcoyaabJbe1cwOhtX4ioEtipY/2/x8Svjynm7HUSc2Wl9Ws8lji MIME-Version: 1.0 X-Received: by 10.42.84.9 with SMTP id j9mr6425878icl.60.1408372501866; Mon, 18 Aug 2014 07:35:01 -0700 (PDT) Received: by 10.50.20.231 with HTTP; Mon, 18 Aug 2014 07:35:01 -0700 (PDT) In-Reply-To: References: Date: Mon, 18 Aug 2014 16:35:01 +0200 Message-ID: Subject: Re: [PATCH] Introduce per-instance filesystem UUIDs From: Stefan Fuhrmann To: Evgeny Kotkov Cc: Subversion Development Content-Type: multipart/alternative; boundary=90e6ba614d48af83fd0500e846f6 X-Virus-Checked: Checked by ClamAV on apache.org --90e6ba614d48af83fd0500e846f6 Content-Type: text/plain; charset=UTF-8 On Fri, Aug 15, 2014 at 12:15 PM, Evgeny Kotkov wrote: > > To be constructive, I implemented the feature in the db/uuid > > instead of db/format - taking your code and commentary > > where possible. That automatically covered the dump / load > > issue and shortened the patch quite a bit. Cache keying > > and structure description update are also addressed. > > > > I did not implement an extension to 'svnadmin setuuid' to only > > update the instance ID because I'm not sure I really want it. > > > > A minor thing I did is to call the new feature 'instance ID' > > instead of 'instance UUID' because there is no need to guarantee > > global uniqueness. Our current implementation just happens > > to generate UUIDs. Future code might use something else. > > But that is a only a minor point. > > Agreed. I like the approach chosen in the V2 patch, so, I did a few > tweaks to > it and committed the patch in r1618138. > I reviewed the diff between my patch and r1618138 and agree with your modifications. > Worth mentioning, I found the "dump / load" part of the log message a bit > misleading. As I see it, we do not really *need* to bump the instance ID > when > going through dump / load procedure. Whenever you load the dump into an > empty > destination, that destination will have a unique instance ID, and nothing > bad > will happen even if that instance ID survives through the svn_fs_set_uuid() > call. However, I am fine with also bumping the instance ID here, because > it > is a bit simpler to implement and kind of fits the whole concept. Well, currently there is no reason (such like keeping caches valid etc.) to keep an instance ID once you bump the UUID. Instance IDs are an internal aspect of the implementation and we are free to change their usage / preservation scheme in later releases. > Presumably, > it is also better in two edge cases that I could think of (from my point of > view, both of them are "broken workflows", but they are still theoretically > possible): > > - Recovering from a disaster by loading an incremental dump into an old > repository snapshot with '--force-uuid', and performing a hot swap after > it. > > - Loading identical dumps (or different dumps, but with '--ignore-uuid') > into an > empty template repository, e.g., a repository with configured hooks and > common > access rights. This workflow assumes that the template repository is > being > copy-pasted prior to loading something into it. > > So, I tweaked this part of the log message accordingly. Hopefully, I am > not > missing something crucial here. I would also like to solve the problem > with > incremental hotcopy not taking the pack-lock on the destination (as > mentioned > earlier), because now this should be fairly trivial. > Here at the SHF hackathon, we briefly talked about how to proceed. I quite happy with the current implementation wrt to its technical approach. However, we identified another operation that should bump the instance ID, fs_recover. And there might be more. None of that is complicated to solve in any way but we should have a full, reviewed list of places that need to update or possibly use the instance ID before releasing it. So, the correct way to to that is having a branch for the full feature - as suggested by Brane. Given that many of us are the same room for this week, progress on that branch as well as its review can be quick. Unless there is something fundamentally broken with it, there is a good chance to have it in 1.9. > P.S. I have reread my previous answers in this thread and I realized that > they > can be considered quite arrogant. :D To me, you came across as fighting desperately to get the change into the 1.9 release. More specifically, you seemed to have a hard time seeing any reason not to just apply that simple enough fix to for specific problem. > That was utterly unnecessary (it never *is* > necessary, actually), so, I am terribly sorry for that. > I really appreciate this. Written communication is the most lossy, opening things to bias and lots of second guessing. I have to remind myself of that fact once in a while. Cheers! -- Stefan^2. --90e6ba614d48af83fd0500e846f6 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
On F= ri, Aug 15, 2014 at 12:15 PM, Evgeny Kotkov <evgeny.kotkov@vis= ualsvn.com> wrote:
> To b= e constructive, I implemented the feature in the db/uuid
> instead of db/format - taking your code and commentary
> where possible. That automatically covered the dump / load
> issue and shortened the patch quite a bit. Cache keying
> and structure description update are also addressed.
>
> I did not implement an extension to 'svnadmin setuuid' to only=
> update the instance ID because I'm not sure I really want it.
>
> A minor thing I did is to call the new feature 'instance ID' > instead of 'instance UUID' because there is no need to guarant= ee
> global uniqueness. Our current implementation just happens
> to generate UUIDs. Future code might use something else.
> But that is a only a minor point.

Agreed. =C2=A0I like the approach chosen in the V2 patch, so, I did a= few tweaks to
it and committed the patch in r1618138.

I reviewed the diff between my patch and r1618138 and
agree with your m= odifications.
=C2=A0
Worth mentioning, I found the "dump / load" part of the log messa= ge a bit
misleading. =C2=A0As I see it, we do not really *need* to bump the instance= ID when
going through dump / load procedure. =C2=A0Whenever you load the dump into = an empty
destination, that destination will have a unique instance ID, and nothing b= ad
will happen even if that instance ID survives through the svn_fs_set_uuid()=
call. =C2=A0However, I am fine with also bumping the instance ID here, beca= use it
is a bit simpler to implement and kind of fits the whole concept.

Well, currently there is no reason (such like keepin= g caches
valid etc.) to keep an instance ID once you bump the UUID.
Instance IDs are an internal aspect of the implementation and
we are free to change their usage / preservation scheme in
later releas= es.
=C2=A0
=C2=A0Presumably,
it is also better in two edge cases that I could think of (from my point of=
view, both of them are "broken workflows", but they are still the= oretically
possible):

- Recovering from a disaster by loading an incremental dump into an old
=C2=A0 repository snapshot with '--force-uuid', and performing a ho= t swap after it.

- Loading identical dumps (or different dumps, but with '--ignore-uuid&= #39;) into an
=C2=A0 empty template repository, e.g., a repository with configured hooks = and common
=C2=A0 access rights. =C2=A0This workflow assumes that the template reposit= ory is being
=C2=A0 copy-pasted prior to loading something into it.

So, I tweaked this part of the log message accordingly. =C2=A0Hopefully, I = am not
missing something crucial here. =C2=A0I would also like to solve the proble= m with
incremental hotcopy not taking the pack-lock on the destination (as mention= ed
earlier), because now this should be fairly trivial.
<= br>
Here at the SHF hackathon, we briefly talked about how to
= proceed. I quite happy with the current implementation wrt
to its technical approach. However, we identified another
operation that= should bump the instance ID, fs_recover.
And there might be = more.

None of that is complicated to solve in any way but= we should
have a full, reviewed list of places that need to update or
<= /div>
possibly use the instance ID before releasing it. So, the
correct way to to that is having a branch for the full feature -
as suggested by Brane.

Given that many of us a= re the same room for this week,
progress on that branch as well as its r= eview can be quick.
Unless there is something fundamentally b= roken with it,
there is a good chance to have it in 1.9.
=C2=A0
P.S. I have reread my previous answers in this thread and I realized that t= hey
can be considered quite arrogant.

:D To me,= you came across as fighting desperately to get
the change into the 1.9 = release. More specifically, you seemed
to have a hard time seeing any re= ason not to just apply that
simple enough fix to for specific problem.
=C2=A0
=C2=A0That was utterly unneces= sary (it never *is*
necessary, actually), so, I am terribly sorry for that.

I really appreciate this. Written communication is the mos= t
lossy, opening things to bias and lots of second guessing.
I have t= o remind myself of that fact once in a while.

Cheers!
-- Stefan^2.

<= /div>
--90e6ba614d48af83fd0500e846f6--