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_at_wandisco.com> wrote:
>
> On Sun, Aug 10, 2014 at 3:56 PM, Evgeny Kotkov <
> evgeny.kotkov_at_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.
Received on 2014-08-12 23:47:36 CEST