You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Evgeny Kotkov <ev...@visualsvn.com> on 2014/08/08 19:24:12 UTC
[PATCH] Introduce per-instance filesystem UUIDs
Hi,
I would like to propose a patch for the problem discussed in
http://svn.haxx.se/dev/archive-2014-04/0245.shtml
Please see the details below.
Log message:
[[[
Avoid shared data clashes between repositories duplicated using 'hotcopy',
see http://svn.haxx.se/dev/archive-2014-04/0245.shtml
This is not a "scalability issue" (as stated in the referenced thread), but
rather a full-fledged problem. We have an ability to share data between
different objects pointing to same filesystems. The sharing works within
a single process boundary; currently we share locks (svn_mutex__t objects)
and certain transaction data. Accessing shared data requires some sort of
a key and currently we use a filesystem UUID for this purpose. However,
this is *not* good enough for at least a couple of scenarios.
Filesystem UUIDs aren't really unique for every filesystem an end user might
have, because they get duplicated during hotcopy or naive copy (copy-paste).
Whenever we have two filesystems with the same UUIDs open within a single
process, the shared data starts clashing and things can get pretty ugly.
For example, one can experience random errors with parallel commits to two
repositories with the same UUID (hosted by Apache HTTP Server). Another
example was recently mitigated by http://svn.apache.org/r1589653 — we did
encounter a deadlock within nested 'svnadmin freeze' commands executed for
two repositories with the same UUID.
Errors that I witnessed include (but might not be limited to):
- Cannot write to the prototype revision file of transaction '392-ax'
because a previous representation is currently being written by this
process (SVN_ERR_FS_CORRUPT)
- Can't unlock unknown transaction '392-ax' (SVN_ERR_FS_CORRUPT)
- Recursive locks are not supported (SVN_ERR_RECURSIVE_LOCK)
# This used to be deadlock prior to http://svn.apache.org/r1591919
Fix the issue by introducing a concept of "instance UUIDs" on the FS layer.
Basically, this gives us an ability to distinguish filesystem duplicates or
near-duplicates produced via our API (svn_fs_hotcopy3(), to be exact). We
can now have different filesystems with the same "original" UUID, but with
different instance UUIDs. With this concept, it is rather easy to get rid
of the shared data clashes described above.
* subversion/libsvn_fs_fs/fs.h
(SVN_FS_FS__MIN_INSTANCE_UUID_FORMAT): New.
(fs_fs_data_t.instance_uuid): New.
* subversion/libsvn_fs_fs/fs_fs.c
(read_format, svn_fs_fs__write_format): Support the new 'instance-uuid'
format option.
(svn_fs_fs__open): Initialize the instance UUID when it is present and
fallback to the original UUID of a filesystem whenever the instance
UUID is absent.
(upgrade_body, svn_fs_fs__create): Generate a new instance UUID when
necessary.
* subversion/libsvn_fs_fs/hotcopy.c
(hotcopy_create_empty_dest): Unconditionally generate a new instance UUID.
* subversion/libsvn_fs_fs/fs.c
(fs_serialized_init): Use a combination of two UUIDs as the shared data
key (see the huge comment block about why we concatenate them).
* subversion/tests/cmdline/svnadmin_tests.py
(check_hotcopy_fsfs_fsx): Allow different 'instance-uuid' format options
when comparing the db/format contents.
(freeze_freeze): Do not change UUID of hotcopy for new formats supporting
instance UUIDs. For new formats, 'svnadmin freeze A (svnadmin freeze B)'
should not deadlock or error out with SVN_ERR_RECURSIVE_LOCK even if 'A'
and 'B' share the same UUID.
* subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c
(write_format): Rework this helper. Before this changeset, we were always
rewriting 'db/format' contents with predefined values. What we really
want here is to switch the given filesystem to a sharded layout with a
specific number of revisions per shard. We might as well achieve this
by patching the 'layout' format option and doing so would not somehow
affect the new 'instance-uuid' format option. Implement this logic,
rename the function into ...
(patch_format): ...this, and, finally ...
(create_packed_filesystem): ...update the only caller.
]]]
Are there any objections to this change?
Regards,
Evgeny Kotkov
Re: [PATCH] Introduce per-instance filesystem UUIDs
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Evgeny Kotkov <ev...@visualsvn.com> writes:
> Ben Reser <be...@reser.org> writes:
>
>>> I think part of the problem here has been we (as in WANdisco folks) have
>>> discussed the idea of an instance ID for repositories in the past to solve
>>> the range of replacing the repository without clearing the cache issues.
>>> But this change is being added for a very different reason.
>>>
>>> Evgeny has implemented the instance ID for the purpose of solving the
>>> problem of two different repositories not being able to be locked if they
>>> happen to have the same UUID. This happens because we use a mutex to handle
>>> locking between threads and that mutex can't distinguish between different
>>> repositories with identical UUIDs.
>>>
>>> Currently the code on trunk adds the instance ID to the cache keys. I'm not
>>> sure we should be doing that (though both brane and stefan2 requested that
>>> be done). As per the discussion today at the SHF hackathon the instance ID
>>> can't resolve the failure to clear the cache issues. The best it can do is
>>> narrow the window for these issues to exist. That would seem like a good
>>> thing but I think it creates a huge false sense of security. We will
>>> ultimately have someone that comes along with a corrupted repository, we're
>>> going to say you replaced the repo while the server was running and the
>>> user is going to say "But I've been doing this for years without any
>>> problem."
>>>
>>> Without the instance ID in the cache keys users are unlikely to actually
>>> corrupt their repository (just like they would be with them, it's a pretty
>>> hard race to hit). But they are likely to get errors related to the cache
>>> being stale. This gives them a giant hint that what they're doing is wrong
>>> and gives us an opportunity to educate them.
>>
>> Evgeny do you have any thoughts on this? I think it's probably best to
>> remove the instance id from the cache keys. I believe brane and stefan2
>> were convinced of this at the hackathon.
>
> Sounds good to me.
>
> However, prior to (re-)narrowing the scope of this change, I would love to
> spend a bit of time and actually reproduce the repository corruption race
> (with or without instance IDs) — just in order to obtain a bit of information,
> which I currently lack. I plan to undo the corresponding part of the
> changeset right after that.
Done in r1623402.
Regards,
Evgeny Kotkov
Re: [PATCH] Introduce per-instance filesystem UUIDs
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Ben Reser <be...@reser.org> writes:
>> I think part of the problem here has been we (as in WANdisco folks) have
>> discussed the idea of an instance ID for repositories in the past to solve
>> the range of replacing the repository without clearing the cache issues.
>> But this change is being added for a very different reason.
>>
>> Evgeny has implemented the instance ID for the purpose of solving the problem
>> of two different repositories not being able to be locked if they happen to
>> have the same UUID. This happens because we use a mutex to handle locking
>> between threads and that mutex can't distinguish between different
>> repositories with identical UUIDs.
>>
>> Currently the code on trunk adds the instance ID to the cache keys. I'm not
>> sure we should be doing that (though both brane and stefan2 requested that be
>> done). As per the discussion today at the SHF hackathon the instance ID
>> can't resolve the failure to clear the cache issues. The best it can do is
>> narrow the window for these issues to exist. That would seem like a good
>> thing but I think it creates a huge false sense of security. We will
>> ultimately have someone that comes along with a corrupted repository, we're
>> going to say you replaced the repo while the server was running and the user
>> is going to say "But I've been doing this for years without any problem."
>>
>> Without the instance ID in the cache keys users are unlikely to actually
>> corrupt their repository (just like they would be with them, it's a pretty
>> hard race to hit). But they are likely to get errors related to the cache
>> being stale. This gives them a giant hint that what they're doing is wrong
>> and gives us an opportunity to educate them.
>
> Evgeny do you have any thoughts on this? I think it's probably best to remove
> the instance id from the cache keys. I believe brane and stefan2 were
> convinced of this at the hackathon.
Sounds good to me.
However, prior to (re-)narrowing the scope of this change, I would love to
spend a bit of time and actually reproduce the repository corruption race (with
or without instance IDs) — just in order to obtain a bit of information, which
I currently lack. I plan to undo the corresponding part of the changeset right
after that.
Regards,
Evgeny Kotkov
Re: [PATCH] Introduce per-instance filesystem UUIDs
Posted by Ben Reser <be...@reser.org>.
On 8/20/14 9:13 AM, Ben Reser wrote:
> I think part of the problem here has been we (as in WANdisco folks) have
> discussed the idea of an instance ID for repositories in the past to solve the
> range of replacing the repository without clearing the cache issues. But this
> change is being added for a very different reason.
>
> Evgeny has implemented the instance ID for the purpose of solving the problem
> of two different repositories not being able to be locked if they happen to
> have the same UUID. This happens because we use a mutex to handle locking
> between threads and that mutex can't distinguish between different repositories
> with identical UUIDs.
>
> Currently the code on trunk adds the instance ID to the cache keys. I'm not
> sure we should be doing that (though both brane and stefan2 requested that be
> done). As per the discussion today at the SHF hackathon the instance ID can't
> resolve the failure to clear the cache issues. The best it can do is narrow
> the window for these issues to exist. That would seem like a good thing but I
> think it creates a huge false sense of security. We will ultimately have
> someone that comes along with a corrupted repository, we're going to say you
> replaced the repo while the server was running and the user is going to say
> "But I've been doing this for years without any problem."
>
> Without the instance ID in the cache keys users are unlikely to actually
> corrupt their repository (just like they would be with them, it's a pretty hard
> race to hit). But they are likely to get errors related to the cache being
> stale. This gives them a giant hint that what they're doing is wrong and gives
> us an opportunity to educate them.
Evgeny do you have any thoughts on this? I think it's probably best to remove
the instance id from the cache keys. I believe brane and stefan2 were
convinced of this at the hackathon.
Re: [PATCH] Introduce per-instance filesystem UUIDs
Posted by Ben Reser <be...@reser.org>.
On 8/19/14 12:07 AM, Branko Čibej wrote:
> I think it's not that simple.
>
> Consider the case where an administrator decides to not use 'svnadmin hotcopy'
> to back up a repository, but instead creates a (LVM) snapshot of the volume and
> uses 'tar' (or 'cp -a') to create the backup.
>
> When such a backup is restored and made active, everything will just work ...
> except that stale caches in svnserve or mor_dav_svn will not be automatically
> invalidated. In other words, the mere introduction of the instance ID does not
> solve "all" problems. There are several possible resolutions to this particular
> problem:
>
> * Tell the users "don't do that". That won't help; they'll do it anyway.
> * Require a restart of all servers when restoring such backups; been there,
> people forget.
> * Require that the users run 'svnadmin recover' before bringing the
> repository online; this might work if 'svnadmin recover' tweaks the
> instance ID, since presumably they're already using it per our existing
> recommendation.
> * Invent 'svnadmin restore' or 'svnadmin activate' or whatnot to make such
> backups viable; see above, people forget.
> * Require 'svnadmin setuuid' on the restored backups; this breaks existing
> working copies.
>
> So, even though the existence of the instance ID is an implementation detail,
> it does cause visible change in the behaviour of the repository: server
> restarts due to fiddling with the repository instance are needed far less
> often; but we still have to document when and why they are needed.
I think part of the problem here has been we (as in WANdisco folks) have
discussed the idea of an instance ID for repositories in the past to solve the
range of replacing the repository without clearing the cache issues. But this
change is being added for a very different reason.
Evgeny has implemented the instance ID for the purpose of solving the problem
of two different repositories not being able to be locked if they happen to
have the same UUID. This happens because we use a mutex to handle locking
between threads and that mutex can't distinguish between different repositories
with identical UUIDs.
Currently the code on trunk adds the instance ID to the cache keys. I'm not
sure we should be doing that (though both brane and stefan2 requested that be
done). As per the discussion today at the SHF hackathon the instance ID can't
resolve the failure to clear the cache issues. The best it can do is narrow
the window for these issues to exist. That would seem like a good thing but I
think it creates a huge false sense of security. We will ultimately have
someone that comes along with a corrupted repository, we're going to say you
replaced the repo while the server was running and the user is going to say
"But I've been doing this for years without any problem."
Without the instance ID in the cache keys users are unlikely to actually
corrupt their repository (just like they would be with them, it's a pretty hard
race to hit). But they are likely to get errors related to the cache being
stale. This gives them a giant hint that what they're doing is wrong and gives
us an opportunity to educate them.
Re: [PATCH] Introduce per-instance filesystem UUIDs
Posted by Branko Čibej <br...@wandisco.com>.
On 18.08.2014 21:43, Evgeny Kotkov wrote:
> What I am really concerned about is that this internal little fix
> could ultimately become an unnecessary complicated, full-stack and
> user-visible feature. I doubt that we want users to know something
> about the way we solve our *internal* problems. Why would they ever
> need to know?
I think it's not that simple.
Consider the case where an administrator decides to not use 'svnadmin
hotcopy' to back up a repository, but instead creates a (LVM) snapshot
of the volume and uses 'tar' (or 'cp -a') to create the backup.
When such a backup is restored and made active, everything will just
work ... except that stale caches in svnserve or mor_dav_svn will not be
automatically invalidated. In other words, the mere introduction of the
instance ID does not solve "all" problems. There are several possible
resolutions to this particular problem:
* Tell the users "don't do that". That won't help; they'll do it anyway.
* Require a restart of all servers when restoring such backups; been
there, people forget.
* Require that the users run 'svnadmin recover' before bringing the
repository online; this might work if 'svnadmin recover' tweaks the
instance ID, since presumably they're already using it per our
existing recommendation.
* Invent 'svnadmin restore' or 'svnadmin activate' or whatnot to make
such backups viable; see above, people forget.
* Require 'svnadmin setuuid' on the restored backups; this breaks
existing working copies.
So, even though the existence of the instance ID is an implementation
detail, it does cause visible change in the behaviour of the repository:
server restarts due to fiddling with the repository instance are needed
far less often; but we still have to document when and why they are needed.
I don't pretend that the cases I listed above cover all the possible
side effects of introducing an instance ID. That's why I asked for a
branch, with documentation about how and why the instance ID affects
existing commands (and APIs) -- even if this documentation is only in a
BRANCH-README file and is later included in the release notes. Also, we
may decide not to implement some of the things (e.g., in svnadmin) that
constitute the "complete set" of instance-id related changes, but before
deciding which of them we can safely skip, we need an idea of what the
complete set is; again, easier to get that idea with a branch to work on
than trying to figure out what we "forgot" to implement on trunk.
IMO, the extra hassle of managing the branch is much smaller than the
extra hassle of releasing 1.9.1..1.9.5 in the first 2 months after
1.9.0, just because we forgot to implement some (in retrospect) obvious
changes on trunk before the release.
I'm sure the 1.8.0 fiasco taught us a thing or two about that; so, let's
try not to repeat it.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane@wandisco.com
Re: [PATCH] Introduce per-instance filesystem UUIDs
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Hi Stefan,
> 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.
[...]
> :D To me, you came across as fighting desperately to get
> the change into the 1.9 release.
Actually, I do not have concerns about this change becoming or not becoming a
part of 1.9. What I am really concerned about is that this internal little fix
could ultimately become an unnecessary complicated, full-stack and user-visible
feature. I doubt that we want users to know something about the way we solve
our *internal* problems. Why would they ever need to know? Probably not the
best analogy, but to me, it would look the same way if we, for instance, added
a command(s) to do some sort of reference counting that Subversion requires
underneath. Or, maybe, to deallocate memory. Or to close unused file handles.
As long as you are together on the hackathon, and everyone thinks that
reverting-and-branching-and-voting is a better way here, I would not insist on
doing it somehow else. However, I suspect that this approach requires a proper
revert justfication, and, unfortunately, I cannot really come up with that sort
of thing myself.
> 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 is true, it became a lot easier with the provided patch.
> 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.
Thank you for these kind words.
Regards,
Evgeny Kotkov
Re: [PATCH] Introduce per-instance filesystem UUIDs
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Aug 15, 2014 at 12:15 PM, Evgeny Kotkov <evgeny.kotkov@visualsvn.com
> 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.
Re: [PATCH] Introduce per-instance filesystem UUIDs
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
> 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.
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. 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.
P.S. I have reread my previous answers in this thread and I realized that they
can be considered quite arrogant. That was utterly unnecessary (it never *is*
necessary, actually), so, I am terribly sorry for that.
Regards,
Evgeny Kotkov
Re: [PATCH] Introduce per-instance filesystem UUIDs
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Aug 12, 2014 at 11:47 PM, Stefan Fuhrmann <
stefan.fuhrmann@wandisco.com> wrote:
> Arrgs!! Random key combo made me sent the mail before completing it ...
>
>
> On Tue, Aug 12, 2014 at 11:29 PM, Stefan Fuhrmann <
> stefan.fuhrmann@wandisco.com> wrote:
>
>>
>> On Sun, Aug 10, 2014 at 3:56 PM, Evgeny Kotkov <
>> evgeny.kotkov@visualsvn.com> wrote:
>>
>
[...]
> I have found myself in the same situation as you before.
> A seemingly simple change causing lots of discussion.
> It's surprising and somewhat disheartening, but in the
> end the code has been better than it would have been
> without the discussions. The key is that everyone needs
> to stay constructive in their criticisms.
>
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.
-- Stefan^2.
Re: [PATCH] Introduce per-instance filesystem UUIDs
Posted by Stefan Fuhrmann <st...@wandisco.com>.
Arrgs!! Random key combo made me sent the mail before completing it ...
On Tue, Aug 12, 2014 at 11:29 PM, Stefan Fuhrmann <
stefan.fuhrmann@wandisco.com> wrote:
>
> On Sun, Aug 10, 2014 at 3:56 PM, Evgeny Kotkov <
> evgeny.kotkov@visualsvn.com> wrote:
>
>> Hi Stefan,
>>
>> > Thanks for having a the (missing) instance ID issue.
>> > From initial review, I have 1 objection and 2 issues
>> > that your patch does not address, yet.
>>
>> Thank you for reviewing this patch. I did my best, but I cannot really
>> tell,
>> which of these points are objections, and which of them are issues with
>> the
>> proposed patch.
>>
>
> No problem, I usually answer questions - eventually ;)
> (It all depends on whether an the answers require
> thought, what my TODO list or travel schedule is etc.)
>
>
>> This patch solves a well-defined problem that was particularly stated in
>> the comments prior to r1589262:
>> [[[
>> Using the uuid to obtain the lock creates a corner case if a
>> caller uses svn_fs_set_uuid on the repository in a process where
>> other threads might be using the same repository through another
>> FS object. The only real-world consumer of svn_fs_set_uuid is
>> "svnadmin load", so this is a low-priority problem, and we don't
>> know of a better way of associating such data with the
>> repository.
>> ]]]
>>
>
> It's perfectly fine to fix that issue. Doing so introduces
> a new feature ("instance ID") and I think the code should
> cover related issues as well. It should take only a few
> extra lines to fix the whole class of related problems.
>
> Just to mention, this comment was not restored in r1589518, although the
>> changeset was claimed to be a "revert".
>
>
> Thanks for spotting it! r1589518 was not a strict revert.
> That's how the old comment fell through the cracks.
> Fixed in r1617586.
>
>
>> And (just to mention #2) without a
>> change like this we *do* have a regression in the incremental hotcopy
>> behavior
>> compared to 1.8, because now the destination is allowed to be packed while
>> hotcopy is in progress, and that's where real "ripple effects" become
>> visible.
>>
>
> That sounds like a separate issue. Hotcopy should take
> out the pack lock on the target as well. Feel free to fix it.
>
>
>> > * Ideally, we would store the instance UUID as a separate
>> > file next to uuid and friends. Presumably trying to not
>> > increase the fs_open latency low, you merged that info
>> > into the format file - causing various ripple effects.
>> > I think it should be put into the UUID file (or a separate
>> > file altogether) and update places where we rewrite
>> > this file (e.g. svnadmin load).
>>
>> Honestly, I do not get this — what exactly are the "ripple effects" you
>> are
>> talking about? Don't you think that adding another file would not have
>> its own
>> "ripple effects", at least considering the amount of places where you
>> have to
>> change the code? And again, I still do not see any real problems with the
>> approach I chose.
>>
>
> The instance ID has nothing to do with format info. Agree?
> Putting it there is just plain wrong.
>
> Putting it into the same file means that you have to filter it
> whenever you want to change or check one but not the other.
> All of your changes in the fs-fs-pack-test were only needed
> for that reason. But these "ripple effects" are only a symptom
> of a bad design choice. Having e.g. a completely separate
> file would not require any changes to the existing code to
> maintain the current functionality.
>
>
>> From my point of view, it is not even a matter of taste. We require the
>> 'instance-uuid' option as a solution to our *internal* problem with the
>> shared
>> data clashes (and, possibly, with outdated cache contents after the
>> repository
>> replacement). Putting a thing like this into the 'db/format' file has at
>> least
>> a couple advantages:
>>
>
>> - You do not have to open yet-another-file within fs_open() calls. These
>> are
>> frequent — think about how often Apache HTTP Server does it. So, why
>> don't
>> we entirely avoid this overhead, considering the fact that it does not
>> make
>> any real difference?
>>
>
> Putting it into the existing uuid file would be just as efficient
> and touch fewer code bits.
>
>
>> - This actually makes sense.
>
>
> No. It is does not tell us anything about the FS format.
>
>
>> We have the filesystem UUID stored in a separate
>> 'db/uuid' file and it can be changed by the end user (svnadmin
>> setuuid). We
>> also have the instance UUID, which is invisible for the user and solves
>> *our*
>> problems. Whenever we use it appropriately in our caches / shared
>> data, the
>> end user should not *ever* want to change the instance UUID, and, to my
>> mind,
>> we should not introduce another user-visible thing and a corresponding
>> set of
>> commands for the administrator to remember.
>>
>
> I'm not 100% sure whether we should expose the
> instance ID to the user or not. The use case that
> I am unsure about is backup (from hotcopy or not).
>
> Technically, we all working copies become invalid
> and 'svnadmin setuuid' makes that explicit. But there
> are certainly users who would like to keep their
> working copies intact. I honestly have no idea what
> the correct approach here is.
>
>
>> Finally, I do not really understand the 'svnadmin load' point. You do
>> not have
>> to somehow interact with an instance UUID whenever you load something
>> into a
>> repository, do you?
>>
>
> See above. As soon as you restore from backup,
> any internal cache is (potentially) invalid now. An
> ID bump (uuid or instance ID) is needed to prevent
> disaster unless you restart your server processes.
>
>
>> > * The instance ID must become part of the cache key.
>> > This is for the hot-restore-from-backup use-case where
>> > a repository gets replaced with an older version of itself
>> > while the server process is kept alive (caches are still hot).
>>
>> This is irrelevant to this patch.
>
>
> I disagree. Although you did not intend to address
> the caching issue, it is basically the flip side of the
> shared data aliasing problem that you try to solve.
>
>
>> As a matter of fact, this patch also does not
>> solve the move tracking problem.
>
>
> How would this even be related to move tracking?
> Do you refer to renaming repositories on disk?
>
>
>> Using this new UUID in our caches seems like a
>> logical next step and I actually had that in mind for implementing.
>
>
> O.k. And that is what review is for: discussing the
> consequences and potentially related issues. I.e.
> sharing insight and ideas; it is *not* intended for blaming people
> for anything.
>
>
>> However,
>> we can solve all these problems one-by-one, considering the fact that
>> this patch
>> is an atomic change solving *one* exact problem.
>>
>
Even with format file issue not being resolved, it would
be perfectly fine to commit this to some branch and go
from there. What would you gain by going to /trunk
directly? The discussion will not end until the issues
have been resolved in any case.
I have found myself in the same situation as you before.
A seemingly simple change causing lots of discussion.
It's surprising and somewhat disheartening, but in the
end the code has been better than it would have been
without the discussions. The key is that everyone needs
to stay constructive in their criticisms.
>> > * svnadmin should have a means to bump the instance ID.
>> > Again, the restore-from-backup use-case.
>>
>> See above. Again, I cannot tell whether this is an issue or an
>> objection, but
>> "should" looks unjustified. As I understand, you are talking about a
>> triple
>> edge case:
>>
>
It is meant as an object on the sense "the patch does
not cover the bits that I think the proposed feature
should cover".
>
>> 1. An administrator backups the repository by copying them (making disk
>> images,
>> etc.) instead of using the 'hotcopy' command we provide.
>>
>> 2. Something goes wrong, and he/she restores the backup by hot swapping
>> the
>> repository *without* restarting the server.
>>
>> 3. Things start to go wrong with the cached data left from the previous
>> repository before the swap happened.
>>
>> There are two ways of solving this — you can restart the server (which is
>> a
>> better choice)
>
>
I used to suggest the same in the past. But it is actually
tricky to do quickly and non-racy at the same time.
You don't want a prolonged service outage to fix an
issue with a single repository.
> or give the repository a brand new UUID with 'svnadmin setuuid'
>> prior to the swap. Hence, I do not see a necessity in another command
>> that
>> would allow us to change the instance UUIDs. Administrators would have to
>> learn about the difference between filesystem and instance UUIDs, which is
>> pretty much unnecessary, because they *do not* have to know or use these
>> knowledge in order to solve the problems with UUID clashes; they already
>> have a way of fixing them.
>>
>
I agree with the sentiment but I'm not sure (as in really
being able to see both side as the potentially right way
to go) whether flipping the repo UUID is what we always
want. I vaguely remember form WD internal discussions
that there were use-cases where flipping the instance ID
seemed the right thing to do.
One minor thing I forgot to list in my initial response:
* Please update the libsvn_fs_fs/structure file.
-- Stefan^2.
Re: [PATCH] Introduce per-instance filesystem UUIDs
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sun, Aug 10, 2014 at 3:56 PM, Evgeny Kotkov <ev...@visualsvn.com>
wrote:
> Hi Stefan,
>
> > Thanks for having a the (missing) instance ID issue.
> > From initial review, I have 1 objection and 2 issues
> > that your patch does not address, yet.
>
> Thank you for reviewing this patch. I did my best, but I cannot really
> tell,
> which of these points are objections, and which of them are issues with the
> proposed patch.
>
No problem, I usually answer questions - eventually ;)
(It all depends on whether an the answers require
thought, what my TODO list or travel schedule is etc.)
> This patch solves a well-defined problem that was particularly stated in
> the comments prior to r1589262:
> [[[
> Using the uuid to obtain the lock creates a corner case if a
> caller uses svn_fs_set_uuid on the repository in a process where
> other threads might be using the same repository through another
> FS object. The only real-world consumer of svn_fs_set_uuid is
> "svnadmin load", so this is a low-priority problem, and we don't
> know of a better way of associating such data with the
> repository.
> ]]]
>
It's perfectly fine to fix that issue. Doing so introduces
a new feature ("instance ID") and I think the code should
cover related issues as well. It should take only a few
extra lines to fix the whole class of related problems.
Just to mention, this comment was not restored in r1589518, although the
> changeset was claimed to be a "revert".
Thanks for spotting it! r1589518 was not a strict revert.
That's how the old comment fell through the cracks.
Fixed in r1617586.
> And (just to mention #2) without a
> change like this we *do* have a regression in the incremental hotcopy
> behavior
> compared to 1.8, because now the destination is allowed to be packed while
> hotcopy is in progress, and that's where real "ripple effects" become
> visible.
>
That sounds like a separate issue. Hotcopy should take
out the pack lock on the target as well. Feel free to fix it.
> > * Ideally, we would store the instance UUID as a separate
> > file next to uuid and friends. Presumably trying to not
> > increase the fs_open latency low, you merged that info
> > into the format file - causing various ripple effects.
> > I think it should be put into the UUID file (or a separate
> > file altogether) and update places where we rewrite
> > this file (e.g. svnadmin load).
>
> Honestly, I do not get this — what exactly are the "ripple effects" you are
> talking about? Don't you think that adding another file would not have
> its own
> "ripple effects", at least considering the amount of places where you have
> to
> change the code? And again, I still do not see any real problems with the
> approach I chose.
>
The instance ID has nothing to do with format info. Agree?
Putting it there is just plain wrong.
Putting it into the same file means that you have to filter it
whenever you want to change or check one but not the other.
All of your changes in the fs-fs-pack-test were only needed
for that reason. But these "ripple effects" are only a symptom
of a bad design choice. Having e.g. a completely separate
file would not require any changes to the existing code to
maintain the current functionality.
> From my point of view, it is not even a matter of taste. We require the
> 'instance-uuid' option as a solution to our *internal* problem with the
> shared
> data clashes (and, possibly, with outdated cache contents after the
> repository
> replacement). Putting a thing like this into the 'db/format' file has at
> least
> a couple advantages:
>
> - You do not have to open yet-another-file within fs_open() calls. These
> are
> frequent — think about how often Apache HTTP Server does it. So, why
> don't
> we entirely avoid this overhead, considering the fact that it does not
> make
> any real difference?
>
Putting it into the existing uuid file would be just as efficient
and touch fewer code bits.
> - This actually makes sense.
No. It is does not tell us anything about the FS format.
> We have the filesystem UUID stored in a separate
> 'db/uuid' file and it can be changed by the end user (svnadmin setuuid).
> We
> also have the instance UUID, which is invisible for the user and solves
> *our*
> problems. Whenever we use it appropriately in our caches / shared data,
> the
> end user should not *ever* want to change the instance UUID, and, to my
> mind,
> we should not introduce another user-visible thing and a corresponding
> set of
> commands for the administrator to remember.
>
I'm not 100% sure whether we should expose the
instance ID to the user or not. The use case that
I am unsure about is backup (from hotcopy or not).
Technically, we all working copies become invalid
and 'svnadmin setuuid' makes that explicit. But there
are certainly users who would like to keep their
working copies intact. I honestly have no idea what
the correct approach here is.
> Finally, I do not really understand the 'svnadmin load' point. You do not
> have
> to somehow interact with an instance UUID whenever you load something into
> a
> repository, do you?
>
See above. As soon as you restore from backup,
any internal cache is (potentially) invalid now. An
ID bump (uuid or instance ID) is needed to prevent
disaster unless you restart your server processes.
> > * The instance ID must become part of the cache key.
> > This is for the hot-restore-from-backup use-case where
> > a repository gets replaced with an older version of itself
> > while the server process is kept alive (caches are still hot).
>
> This is irrelevant to this patch.
I disagree. Although you did not intend to address
the caching issue, it is basically the flip side of the
shared data aliasing problem that you try to solve.
> As a matter of fact, this patch also does not
> solve the move tracking problem.
How would this even be related to move tracking?
Do you refer to renaming repositories on disk?
> Using this new UUID in our caches seems like a
> logical next step and I actually had that in mind for implementing.
O.k. And that is what review is for: discussing the
consequences and potentially related issues. I.e.
sharing insight and ideas; it is *not* intended for blaming people
for anything.
> However,
> we can solve all these problems one-by-one, considering the fact that this
> patch
> is an atomic change solving *one* exact problem.
>
> > * svnadmin should have a means to bump the instance ID.
> > Again, the restore-from-backup use-case.
>
> See above. Again, I cannot tell whether this is an issue or an objection,
> but
> "should" looks unjustified. As I understand, you are talking about a
> triple
> edge case:
>
> 1. An administrator backups the repository by copying them (making disk
> images,
> etc.) instead of using the 'hotcopy' command we provide.
>
> 2. Something goes wrong, and he/she restores the backup by hot swapping the
> repository *without* restarting the server.
>
> 3. Things start to go wrong with the cached data left from the previous
> repository before the swap happened.
>
> There are two ways of solving this — you can restart the server (which is a
> better choice) or give the repository a brand new UUID with 'svnadmin
> setuuid'
> prior to the swap. Hence, I do not see a necessity in another command that
> would allow us to change the instance UUIDs. Administrators would have to
> learn about the difference between filesystem and instance UUIDs, which is
> pretty much unnecessary, because they *do not* have to know or use these
> knowledge in order to solve the problems with UUID clashes; they already
> have a way of fixing them.
>
>
> Regards,
> Evgeny Kotkov
>
Re: [PATCH] Introduce per-instance filesystem UUIDs
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Hi Stefan,
> Thanks for having a the (missing) instance ID issue.
> From initial review, I have 1 objection and 2 issues
> that your patch does not address, yet.
Thank you for reviewing this patch. I did my best, but I cannot really tell,
which of these points are objections, and which of them are issues with the
proposed patch.
This patch solves a well-defined problem that was particularly stated in
the comments prior to r1589262:
[[[
Using the uuid to obtain the lock creates a corner case if a
caller uses svn_fs_set_uuid on the repository in a process where
other threads might be using the same repository through another
FS object. The only real-world consumer of svn_fs_set_uuid is
"svnadmin load", so this is a low-priority problem, and we don't
know of a better way of associating such data with the
repository.
]]]
Just to mention, this comment was not restored in r1589518, although the
changeset was claimed to be a "revert". And (just to mention #2) without a
change like this we *do* have a regression in the incremental hotcopy behavior
compared to 1.8, because now the destination is allowed to be packed while
hotcopy is in progress, and that's where real "ripple effects" become visible.
> * Ideally, we would store the instance UUID as a separate
> file next to uuid and friends. Presumably trying to not
> increase the fs_open latency low, you merged that info
> into the format file - causing various ripple effects.
> I think it should be put into the UUID file (or a separate
> file altogether) and update places where we rewrite
> this file (e.g. svnadmin load).
Honestly, I do not get this — what exactly are the "ripple effects" you are
talking about? Don't you think that adding another file would not have its own
"ripple effects", at least considering the amount of places where you have to
change the code? And again, I still do not see any real problems with the
approach I chose.
>From my point of view, it is not even a matter of taste. We require the
'instance-uuid' option as a solution to our *internal* problem with the shared
data clashes (and, possibly, with outdated cache contents after the repository
replacement). Putting a thing like this into the 'db/format' file has at least
a couple advantages:
- You do not have to open yet-another-file within fs_open() calls. These are
frequent — think about how often Apache HTTP Server does it. So, why don't
we entirely avoid this overhead, considering the fact that it does not make
any real difference?
- This actually makes sense. We have the filesystem UUID stored in a separate
'db/uuid' file and it can be changed by the end user (svnadmin setuuid). We
also have the instance UUID, which is invisible for the user and solves *our*
problems. Whenever we use it appropriately in our caches / shared data, the
end user should not *ever* want to change the instance UUID, and, to my mind,
we should not introduce another user-visible thing and a corresponding set of
commands for the administrator to remember.
Finally, I do not really understand the 'svnadmin load' point. You do not have
to somehow interact with an instance UUID whenever you load something into a
repository, do you?
> * The instance ID must become part of the cache key.
> This is for the hot-restore-from-backup use-case where
> a repository gets replaced with an older version of itself
> while the server process is kept alive (caches are still hot).
This is irrelevant to this patch. As a matter of fact, this patch also does not
solve the move tracking problem. Using this new UUID in our caches seems like a
logical next step and I actually had that in mind for implementing. However,
we can solve all these problems one-by-one, considering the fact that this patch
is an atomic change solving *one* exact problem.
> * svnadmin should have a means to bump the instance ID.
> Again, the restore-from-backup use-case.
See above. Again, I cannot tell whether this is an issue or an objection, but
"should" looks unjustified. As I understand, you are talking about a triple
edge case:
1. An administrator backups the repository by copying them (making disk images,
etc.) instead of using the 'hotcopy' command we provide.
2. Something goes wrong, and he/she restores the backup by hot swapping the
repository *without* restarting the server.
3. Things start to go wrong with the cached data left from the previous
repository before the swap happened.
There are two ways of solving this — you can restart the server (which is a
better choice) or give the repository a brand new UUID with 'svnadmin setuuid'
prior to the swap. Hence, I do not see a necessity in another command that
would allow us to change the instance UUIDs. Administrators would have to
learn about the difference between filesystem and instance UUIDs, which is
pretty much unnecessary, because they *do not* have to know or use these
knowledge in order to solve the problems with UUID clashes; they already
have a way of fixing them.
Regards,
Evgeny Kotkov
Re: [PATCH] Introduce per-instance filesystem UUIDs
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Aug 8, 2014 at 7:24 PM, Evgeny Kotkov <ev...@visualsvn.com>
wrote:
> Hi,
>
> I would like to propose a patch for the problem discussed in
> http://svn.haxx.se/dev/archive-2014-04/0245.shtml
>
> Please see the details below.
>
[...]
Hi Evgeny,
Thanks for having a the (missing) instance ID issue.
>From initial review, I have 1 objection and 2 issues
that your patch does not address, yet.
* Ideally, we would store the instance UUID as a separate
file next to uuid and friends. Presumably trying to not
increase the fs_open latency low, you merged that info
into the format file - causing various ripple effects.
I think it should be put into the UUID file (or a separate
file altogether) and update places where we rewrite
this file (e.g. svnadmin load).
* The instance ID must become part of the cache key.
This is for the hot-restore-from-backup use-case where
a repository gets replaced with an older version of itself
while the server process is kept alive (caches are still hot).
* svnadmin should have a means to bump the instance ID.
Again, the restore-from-backup use-case.
All these bit can be developed and reviewed neatly on
a branch.
-- Stefan^2.
Re: [PATCH] Introduce per-instance filesystem UUIDs
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 8 August 2014 21:24, Evgeny Kotkov <ev...@visualsvn.com> wrote:
> Hi,
>
> I would like to propose a patch for the problem discussed in
> http://svn.haxx.se/dev/archive-2014-04/0245.shtml
>
> Please see the details below.
>
> Log message:
> [[[
> Avoid shared data clashes between repositories duplicated using 'hotcopy',
> see http://svn.haxx.se/dev/archive-2014-04/0245.shtml
>
> This is not a "scalability issue" (as stated in the referenced thread), but
> rather a full-fledged problem. We have an ability to share data between
> different objects pointing to same filesystems. The sharing works within
> a single process boundary; currently we share locks (svn_mutex__t objects)
> and certain transaction data. Accessing shared data requires some sort of
> a key and currently we use a filesystem UUID for this purpose. However,
> this is *not* good enough for at least a couple of scenarios.
>
> Filesystem UUIDs aren't really unique for every filesystem an end user might
> have, because they get duplicated during hotcopy or naive copy (copy-paste).
> Whenever we have two filesystems with the same UUIDs open within a single
> process, the shared data starts clashing and things can get pretty ugly.
> For example, one can experience random errors with parallel commits to two
> repositories with the same UUID (hosted by Apache HTTP Server). Another
> example was recently mitigated by http://svn.apache.org/r1589653 — we did
> encounter a deadlock within nested 'svnadmin freeze' commands executed for
> two repositories with the same UUID.
>
> Errors that I witnessed include (but might not be limited to):
>
> - Cannot write to the prototype revision file of transaction '392-ax'
> because a previous representation is currently being written by this
> process (SVN_ERR_FS_CORRUPT)
>
> - Can't unlock unknown transaction '392-ax' (SVN_ERR_FS_CORRUPT)
>
> - Recursive locks are not supported (SVN_ERR_RECURSIVE_LOCK)
> # This used to be deadlock prior to http://svn.apache.org/r1591919
>
> Fix the issue by introducing a concept of "instance UUIDs" on the FS layer.
> Basically, this gives us an ability to distinguish filesystem duplicates or
> near-duplicates produced via our API (svn_fs_hotcopy3(), to be exact). We
> can now have different filesystems with the same "original" UUID, but with
> different instance UUIDs. With this concept, it is rather easy to get rid
> of the shared data clashes described above.
>
> * subversion/libsvn_fs_fs/fs.h
> (SVN_FS_FS__MIN_INSTANCE_UUID_FORMAT): New.
> (fs_fs_data_t.instance_uuid): New.
>
> * subversion/libsvn_fs_fs/fs_fs.c
> (read_format, svn_fs_fs__write_format): Support the new 'instance-uuid'
> format option.
> (svn_fs_fs__open): Initialize the instance UUID when it is present and
> fallback to the original UUID of a filesystem whenever the instance
> UUID is absent.
> (upgrade_body, svn_fs_fs__create): Generate a new instance UUID when
> necessary.
>
> * subversion/libsvn_fs_fs/hotcopy.c
> (hotcopy_create_empty_dest): Unconditionally generate a new instance UUID.
>
> * subversion/libsvn_fs_fs/fs.c
> (fs_serialized_init): Use a combination of two UUIDs as the shared data
> key (see the huge comment block about why we concatenate them).
>
> * subversion/tests/cmdline/svnadmin_tests.py
> (check_hotcopy_fsfs_fsx): Allow different 'instance-uuid' format options
> when comparing the db/format contents.
> (freeze_freeze): Do not change UUID of hotcopy for new formats supporting
> instance UUIDs. For new formats, 'svnadmin freeze A (svnadmin freeze B)'
> should not deadlock or error out with SVN_ERR_RECURSIVE_LOCK even if 'A'
> and 'B' share the same UUID.
>
> * subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c
> (write_format): Rework this helper. Before this changeset, we were always
> rewriting 'db/format' contents with predefined values. What we really
> want here is to switch the given filesystem to a sharded layout with a
> specific number of revisions per shard. We might as well achieve this
> by patching the 'layout' format option and doing so would not somehow
> affect the new 'instance-uuid' format option. Implement this logic,
> rename the function into ...
> (patch_format): ...this, and, finally ...
> (create_packed_filesystem): ...update the only caller.
> ]]]
>
> Are there any objections to this change?
>
Hi Evgeny,
I've reviewed your patch and I'm +1 to commit: it is clear and fixes
one of fundamental FSFS problems. The most complicated part of the
patch is test suite changes which is good sign :)
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: [PATCH] Introduce per-instance filesystem UUIDs
Posted by Branko Čibej <br...@wandisco.com>.
On 09.08.2014 21:53, Evgeny Kotkov wrote:
>> The idea of introducing a separate repository UUID that is "guaranteed" to
>> be unique has been discussed a couple times, tough maybe not on-list. I
>> agree that we need something like this, for the reasons you state below.
> [...]
>
>> I suggest you create a branch and commit this there, so that people can
>> actually test with different scenarios. If and when you feel that the branch
>> is ready, just initiate a merge-vote on this list — see the recent vote for
>> merging the svn-auth-x509 branch as an example.
>>
>> I think the change is serious enough that it merits being developed on a
>> branch, especially since I'd really like to stabilize trunk and begin the
>> 1.9 release cycle. This could mean that your change wouldn't make it into
>> 1.9.
> I am sorry, but did you look at the patch? Seriously, why do we need a branch
> for a change with a core part that fits into a screen? And what is wrong with
> testing different scenarios after "svn patch"-ing the trunk?
>
> We know that there is a problem, and here is a patch that solves it. It is not
> like I would send some sort of a work-in-progress to this mailing list with a
> [PATCH] tag in the title; it's ready to be committed. Based on my judgement,
> this patch is good, and I actually wanted to commit this directly. However,
> I also want to avoid any possible kind of bias here, and be sure that a change
> like this does not remain unnoticed.
>
> Do you have any sort of objections for this patch as solution for the defined
> problem?
See Stefan's mail. It may be a small change in terms of the size of the
patch, but it's a major new feature of the repository back-end. We
decided a while ago that we prefer to develop and validate those on
branches, especially now that we're (hopefully) close to a release.
Clearly working on a branch is easier than hoping that the current patch
is complete (which it obviously isn't).
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane@wandisco.com
Re: [PATCH] Introduce per-instance filesystem UUIDs
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
> The idea of introducing a separate repository UUID that is "guaranteed" to
> be unique has been discussed a couple times, tough maybe not on-list. I
> agree that we need something like this, for the reasons you state below.
[...]
> I suggest you create a branch and commit this there, so that people can
> actually test with different scenarios. If and when you feel that the branch
> is ready, just initiate a merge-vote on this list — see the recent vote for
> merging the svn-auth-x509 branch as an example.
>
> I think the change is serious enough that it merits being developed on a
> branch, especially since I'd really like to stabilize trunk and begin the
> 1.9 release cycle. This could mean that your change wouldn't make it into
> 1.9.
I am sorry, but did you look at the patch? Seriously, why do we need a branch
for a change with a core part that fits into a screen? And what is wrong with
testing different scenarios after "svn patch"-ing the trunk?
We know that there is a problem, and here is a patch that solves it. It is not
like I would send some sort of a work-in-progress to this mailing list with a
[PATCH] tag in the title; it's ready to be committed. Based on my judgement,
this patch is good, and I actually wanted to commit this directly. However,
I also want to avoid any possible kind of bias here, and be sure that a change
like this does not remain unnoticed.
Do you have any sort of objections for this patch as solution for the defined
problem?
Regards,
Evgeny Kotkov
Re: [PATCH] Introduce per-instance filesystem UUIDs
Posted by Branko Čibej <br...@wandisco.com>.
On 08.08.2014 19:24, Evgeny Kotkov wrote:
> Hi,
>
> I would like to propose a patch for the problem discussed in
> http://svn.haxx.se/dev/archive-2014-04/0245.shtml
>
> Please see the details below.
>
> Log message:
> [[[
> Avoid shared data clashes between repositories duplicated using 'hotcopy',
> see http://svn.haxx.se/dev/archive-2014-04/0245.shtml
[...]
The idea of introducing a separate repository UUID that is "guaranteed"
to be unique has been discussed a couple times, tough maybe not on-list.
I agree that we need something like this, for the reasons you state below.
(N.B: Only "guaranteed", not actually guaranteed, because people can
still do silly things like 'svnadmin freeze repo cp -a repo newrepo'.)
[...]
> Index: subversion/libsvn_fs_fs/fs.h
> =======================================
> --- subversion/libsvn_fs_fs/fs.h (revision 1616822)
> +++ subversion/libsvn_fs_fs/fs.h (working copy)
> @@ -179,7 +179,10 @@
> extern "C" {
> /* Minimum format number that stores mergeinfo-mode flag in changed
> paths */
> #define SVN_FS_FS__MIN_MERGEINFO_IN_CHANGES_FORMAT 7
> +/* Minimum format number that supports per-instance filesystem UUIDs. */
> +#define SVN_FS_FS__MIN_INSTANCE_UUID_FORMAT 7
> +
> /* The minimum format number that supports a configuration file
> (fsfs.conf) */
> #define SVN_FS_FS__MIN_CONFIG_FILE 4
I suggest you create a branch and commit this there, so that people can
actually test with different scenarios. If and when you feel that the
branch is ready, just initiate a merge-vote on this list — see the
recent vote for merging the svn-auth-x509 branch as an example.
I think the change is serious enough that it merits being developed on a
branch, especially since I'd really like to stabilize trunk and begin
the 1.9 release cycle. This could mean that your change wouldn't make it
into 1.9.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane@wandisco.com