You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "C. Michael Pilato" <cm...@collab.net> on 2011/04/13 03:14:25 UTC
Layering violations in the FS cache code? (was "svn commit: r1091573
- /subversion/trunk/build.conf")
On 04/12/2011 05:11 PM, arfrever@apache.org wrote:
> Author: arfrever
> Date: Tue Apr 12 21:11:46 2011
> New Revision: 1091573
>
> URL: http://svn.apache.org/viewvc?rev=1091573&view=rev
> Log:
> Followup to r1090784:
>
> * build.conf
> (libsvn_ra.libs): Add libsvn_fs_util.
Whoa. This doesn't feel right to me. Well, let me clarify, as I can see
why r1090784 makes this necessary.
It's r1090784 which looks severely problematic to me. The RA loader module
is not permitted to access svn_fs_ functions directly! It may only call the
vtable functions provided by the individual RA plugins. Of those plugins,
ra_local is the only one permitted to call svn_fs_ functions.
And now that I'm looking at the svn_fs_get_cache_config(), I'm feeling wrong
about that, too. If this cache stuff is an FSFS-only thing, then it needs
to live inside libsvn_fs_fs. libsvn_fs_util exists *only* for use by FS
implementations (libsvn_fs_fs and libsvn_fs_base, today) which need to share
utility functions.
Sorry I didn't catch this stuff earlier (yes, guilty of "well, *someone*
must be reviewing this code" syndrome), but I'm seeing what appears to be a
series of screaming layering violations here.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: Layering violations in the FS cache code? (was "svn commit: r1091573
- /subversion/trunk/build.conf")
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 04/20/2011 07:40 PM, Stefan Fuhrmann wrote:
> Literally main(). Setting up caching limits etc. is similar
> to creating the apr root pool and limiting its allocator -
> which is done inside main().
>
> Also, this is clearly tool-specific as svn.exe seems to be
> the only tool that would want to disable the membuffer
> cache. A central function like svn_cmdline_init() would
> be inappropriate.
>
> -- Stefan^2.
Okey dokey. I'll watch for your commit, then. ;-)
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: Layering violations in the FS cache code? (was "svn commit: r1091573
- /subversion/trunk/build.conf")
Posted by Stefan Fuhrmann <eq...@web.de>.
On 18.04.2011 21:09, C. Michael Pilato wrote:
> On 04/16/2011 08:40 AM, Stefan Fuhrmann wrote:
>> On 14.04.2011 22:32, Branko Čibej wrote:
>>> On 14.04.2011 21:03, Daniel Shahaf wrote:
>>>> On Thu, 14 Apr 2011 14:43 -0400, "C. Michael Pilato"<cm...@collab.net> wrote:
>>>>> I'm honestly not quite sure exactly where the right place is. I don't
>>>>> really see what moving it to libsvn_fs does for us -- IMO, it's still
>>>>> wrong for svn_ra_initialize() call into that. (libsvn_ra should only
>>>>> call into libsvn_subr, libsvn_delta, and its own RA provider vtable.)
>>>> Would svn_cmdline_init() be a better place to call the init
>>>> function from?
>>> Clearly not, since that function is a startup helper for command-line
>>> programs, not for libraries.
>> There seem to be two separate questions to answer:
>> (1) Where to put the functionality?
>> (2) Where to call it?
>>
>> I get the impression that there is some consensus that
>> the answer to (1) is libsvn_subr.
> Works for me.
>
>> The answer to the second one is more difficult. To default-
>> override the default settings this must be done early, i.e.
>> before they will be evaluated / used. And clients like TSVN
>> must be able to change the settings after the client libs
>> changed the defaults and before the settings get used.
>>
>> Since the changed default is only relevant for time-critical
>> clients, i.e. those being run many times automatically, e.g.
>> as part of some script. IMHO, it would be fine to simply
>> move the override code to the SVN.EXE main(). This would
>> also make it symmetric to svnadmin and svnserve.
> So ... do you literally mean main(), or are you back to the suggestion of
> using svn_cmdline_init()?
Literally main(). Setting up caching limits etc. is similar
to creating the apr root pool and limiting its allocator -
which is done inside main().
Also, this is clearly tool-specific as svn.exe seems to be
the only tool that would want to disable the membuffer
cache. A central function like svn_cmdline_init() would
be inappropriate.
-- Stefan^2.
Re: Layering violations in the FS cache code? (was "svn commit: r1091573
- /subversion/trunk/build.conf")
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 04/16/2011 08:40 AM, Stefan Fuhrmann wrote:
> On 14.04.2011 22:32, Branko Čibej wrote:
>> On 14.04.2011 21:03, Daniel Shahaf wrote:
>>> On Thu, 14 Apr 2011 14:43 -0400, "C. Michael Pilato"<cm...@collab.net> wrote:
>>>> I'm honestly not quite sure exactly where the right place is. I don't
>>>> really see what moving it to libsvn_fs does for us -- IMO, it's still
>>>> wrong for svn_ra_initialize() call into that. (libsvn_ra should only
>>>> call into libsvn_subr, libsvn_delta, and its own RA provider vtable.)
>>>
>>> Would svn_cmdline_init() be a better place to call the init
>>> function from?
>>
>> Clearly not, since that function is a startup helper for command-line
>> programs, not for libraries.
>
> There seem to be two separate questions to answer:
> (1) Where to put the functionality?
> (2) Where to call it?
>
> I get the impression that there is some consensus that
> the answer to (1) is libsvn_subr.
Works for me.
> The answer to the second one is more difficult. To default-
> override the default settings this must be done early, i.e.
> before they will be evaluated / used. And clients like TSVN
> must be able to change the settings after the client libs
> changed the defaults and before the settings get used.
>
> Since the changed default is only relevant for time-critical
> clients, i.e. those being run many times automatically, e.g.
> as part of some script. IMHO, it would be fine to simply
> move the override code to the SVN.EXE main(). This would
> also make it symmetric to svnadmin and svnserve.
So ... do you literally mean main(), or are you back to the suggestion of
using svn_cmdline_init()?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: Layering violations in the FS cache code? (was "svn commit: r1091573
- /subversion/trunk/build.conf")
Posted by Stefan Fuhrmann <eq...@web.de>.
On 14.04.2011 22:32, Branko Čibej wrote:
> On 14.04.2011 21:03, Daniel Shahaf wrote:
>> On Thu, 14 Apr 2011 14:43 -0400, "C. Michael Pilato"<cm...@collab.net> wrote:
>>> On 04/13/2011 06:52 PM, Stefan Fuhrmann wrote:
>>>> The code in question has evolved over many months so it is
>>>> very possible that the name of svn_fs_get_cache_config()
>>>> and friends is no longer appropriate. The same goes for the
>>>> place that this has been implemented.
>>>>
>>>> From a design perspective, I think it is perfectly to o.k. to
>>>> expose resource limits on the server UI level. The fact that FSFS
>>>> is currently the only part of the server that uses these settings
>>>> does not change, IMO, the fact that they are part of the UI.
>>> I don't have an opinion about exposing resource limits via the server
>>> UI. I do have a strong opinion about the code honoring the flow of the
>>> layered library design we have.
I understand that and share that POV. We just stumbled
across some functionality that is somewhat hard to sort.
>>> If you want to tweak server settings
>>> from client code, you need to do so via the RA vtable(), implementing
>>> the logic to do so (or to expressed *not* do so) in every RA provider.
RA vtable would not be the right location because these
settings are global for the server and not session specific.
Again, this is my mistake because I picked RA init code
to tweak the default resource settings. The "clever" part
about that was that it will only influence the *local* process,
i.e. will only affect ra_local data access.
>>> On 04/13/2011 06:52 PM, Stefan Fuhrmann wrote:
>>>> Maybe we should simply move the function in question to libsvn_subr
>>>> and rename them properly.
>>> On 04/13/2011 11:12 PM, Daniel Shahaf wrote:
>>>> I assumed the resolution would be to move that function to libsvn_fs, not
>>>> to libsvn_subr...
>>> I'm honestly not quite sure exactly where the right place is. I don't
>>> really see what moving it to libsvn_fs does for us -- IMO, it's still
>>> wrong for svn_ra_initialize() call into that. (libsvn_ra should only
>>> call into libsvn_subr, libsvn_delta, and its own RA provider vtable.)
>> Would svn_cmdline_init() be a better place to call the init
>> function from?
> Clearly not, since that function is a startup helper for command-line
> programs, not for libraries.
There seem to be two separate questions to answer:
(1) Where to put the functionality?
(2) Where to call it?
I get the impression that there is some consensus that
the answer to (1) is libsvn_subr.
The answer to the second one is more difficult. To default-
override the default settings this must be done early, i.e.
before they will be evaluated / used. And clients like TSVN
must be able to change the settings after the client libs
changed the defaults and before the settings get used.
Since the changed default is only relevant for time-critical
clients, i.e. those being run many times automatically, e.g.
as part of some script. IMHO, it would be fine to simply
move the override code to the SVN.EXE main(). This would
also make it symmetric to svnadmin and svnserve.
-- Stefan^2.
Re: Layering violations in the FS cache code? (was "svn commit: r1091573
- /subversion/trunk/build.conf")
Posted by Branko Čibej <br...@e-reka.si>.
On 14.04.2011 21:03, Daniel Shahaf wrote:
> On Thu, 14 Apr 2011 14:43 -0400, "C. Michael Pilato" <cm...@collab.net> wrote:
>> On 04/13/2011 06:52 PM, Stefan Fuhrmann wrote:
>>> The code in question has evolved over many months so it is
>>> very possible that the name of svn_fs_get_cache_config()
>>> and friends is no longer appropriate. The same goes for the
>>> place that this has been implemented.
>>>
>>> From a design perspective, I think it is perfectly to o.k. to
>>> expose resource limits on the server UI level. The fact that FSFS
>>> is currently the only part of the server that uses these settings
>>> does not change, IMO, the fact that they are part of the UI.
>> I don't have an opinion about exposing resource limits via the server
>> UI. I do have a strong opinion about the code honoring the flow of the
>> layered library design we have. If you want to tweak server settings
>> from client code, you need to do so via the RA vtable(), implementing
>> the logic to do so (or to expressed *not* do so) in every RA provider.
>>
>> On 04/13/2011 06:52 PM, Stefan Fuhrmann wrote:
>>> Maybe we should simply move the function in question to libsvn_subr
>>> and rename them properly.
>> On 04/13/2011 11:12 PM, Daniel Shahaf wrote:
>>> I assumed the resolution would be to move that function to libsvn_fs, not
>>> to libsvn_subr...
>> I'm honestly not quite sure exactly where the right place is. I don't
>> really see what moving it to libsvn_fs does for us -- IMO, it's still
>> wrong for svn_ra_initialize() call into that. (libsvn_ra should only
>> call into libsvn_subr, libsvn_delta, and its own RA provider vtable.)
> Would svn_cmdline_init() be a better place to call the init
> function from?
Clearly not, since that function is a startup helper for command-line
programs, not for libraries.
-- Brane
Re: Layering violations in the FS cache code? (was "svn commit:
r1091573 - /subversion/trunk/build.conf")
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
On Thu, 14 Apr 2011 14:43 -0400, "C. Michael Pilato" <cm...@collab.net> wrote:
> On 04/13/2011 06:52 PM, Stefan Fuhrmann wrote:
> > The code in question has evolved over many months so it is
> > very possible that the name of svn_fs_get_cache_config()
> > and friends is no longer appropriate. The same goes for the
> > place that this has been implemented.
> >
> > From a design perspective, I think it is perfectly to o.k. to
> > expose resource limits on the server UI level. The fact that FSFS
> > is currently the only part of the server that uses these settings
> > does not change, IMO, the fact that they are part of the UI.
>
> I don't have an opinion about exposing resource limits via the server
> UI. I do have a strong opinion about the code honoring the flow of the
> layered library design we have. If you want to tweak server settings
> from client code, you need to do so via the RA vtable(), implementing
> the logic to do so (or to expressed *not* do so) in every RA provider.
>
> On 04/13/2011 06:52 PM, Stefan Fuhrmann wrote:
> > Maybe we should simply move the function in question to libsvn_subr
> > and rename them properly.
>
> On 04/13/2011 11:12 PM, Daniel Shahaf wrote:
> > I assumed the resolution would be to move that function to libsvn_fs, not
> > to libsvn_subr...
>
> I'm honestly not quite sure exactly where the right place is. I don't
> really see what moving it to libsvn_fs does for us -- IMO, it's still
> wrong for svn_ra_initialize() call into that. (libsvn_ra should only
> call into libsvn_subr, libsvn_delta, and its own RA provider vtable.)
Would svn_cmdline_init() be a better place to call the init
function from?
But, wait, svn_cmdline_* live in libsvn_subr, so they can't call
functions implemented in libsvn_fs (not without another layer of
indirection).
Arrgh.
> If we move it all to libsvn_subr -- the common-most utility function
> we have to offer -- I would suppose that that would solve this. But
> again, I don't understand the code, it usages, or its requirements in
> terms of thread-safety and such.
Re: Layering violations in the FS cache code? (was "svn commit: r1091573
- /subversion/trunk/build.conf")
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 04/13/2011 06:52 PM, Stefan Fuhrmann wrote:
> The code in question has evolved over many months so it is
> very possible that the name of svn_fs_get_cache_config()
> and friends is no longer appropriate. The same goes for the
> place that this has been implemented.
>
> From a design perspective, I think it is perfectly to o.k. to
> expose resource limits on the server UI level. The fact that FSFS
> is currently the only part of the server that uses these settings
> does not change, IMO, the fact that they are part of the UI.
I don't have an opinion about exposing resource limits via the server UI. I
do have a strong opinion about the code honoring the flow of the layered
library design we have. If you want to tweak server settings from client
code, you need to do so via the RA vtable(), implementing the logic to do so
(or to expressed *not* do so) in every RA provider.
On 04/13/2011 06:52 PM, Stefan Fuhrmann wrote:
> Maybe we should simply move the function in question to libsvn_subr
> and rename them properly.
On 04/13/2011 11:12 PM, Daniel Shahaf wrote:
> I assumed the resolution would be to move that function to libsvn_fs, not
> to libsvn_subr...
I'm honestly not quite sure exactly where the right place is. I don't
really see what moving it to libsvn_fs does for us -- IMO, it's still wrong
for svn_ra_initialize() call into that. (libsvn_ra should only call into
libsvn_subr, libsvn_delta, and its own RA provider vtable.) If we move it
all to libsvn_subr -- the common-most utility function we have to offer -- I
would suppose that that would solve this. But again, I don't understand the
code, it usages, or its requirements in terms of thread-safety and such.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: Layering violations in the FS cache code? (was "svn commit:
r1091573 - /subversion/trunk/build.conf")
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
On Thu, 14 Apr 2011 00:52 +0200, "Stefan Fuhrmann" <st...@alice-dsl.de> wrote:
> On 13.04.2011 03:14, C. Michael Pilato wrote:
> > On 04/12/2011 05:11 PM, arfrever@apache.org wrote:
> >> Author: arfrever
> >> Date: Tue Apr 12 21:11:46 2011
> >> New Revision: 1091573
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1091573&view=rev
> >> Log:
> >> Followup to r1090784:
> >>
> >> * build.conf
> >> (libsvn_ra.libs): Add libsvn_fs_util.
> > Whoa. This doesn't feel right to me. Well, let me clarify, as I can see
> > why r1090784 makes this necessary.
> >
> > It's r1090784 which looks severely problematic to me. The RA loader module
> > is not permitted to access svn_fs_ functions directly! It may only call the
> > vtable functions provided by the individual RA plugins. Of those plugins,
> > ra_local is the only one permitted to call svn_fs_ functions.
> >
> > And now that I'm looking at the svn_fs_get_cache_config(), I'm feeling wrong
> > about that, too. If this cache stuff is an FSFS-only thing, then it needs
> > to live inside libsvn_fs_fs. libsvn_fs_util exists *only* for use by FS
> > implementations (libsvn_fs_fs and libsvn_fs_base, today) which need to share
> > utility functions.
> >
> > Sorry I didn't catch this stuff earlier (yes, guilty of "well, *someone*
> > must be reviewing this code" syndrome), but I'm seeing what appears to be a
> > series of screaming layering violations here.
> The code in question has evolved over many months so it is
> very possible that the name of svn_fs_get_cache_config()
> and friends is no longer appropriate. The same goes for the
> place that this has been implemented.
...
> Maybe we should simply move the function in question to libsvn_subr
> and rename them properly.
>
> Comments?
>
I assumed the resolution would be to move that function to libsvn_fs, not to libsvn_subr...
But, +1 for moving it to an appropriate place, and reverting the consequential build.conf change.
> -- Stefan^2.
>
Re: Layering violations in the FS cache code? (was "svn commit: r1091573
- /subversion/trunk/build.conf")
Posted by Stefan Fuhrmann <st...@alice-dsl.de>.
On 13.04.2011 03:14, C. Michael Pilato wrote:
> On 04/12/2011 05:11 PM, arfrever@apache.org wrote:
>> Author: arfrever
>> Date: Tue Apr 12 21:11:46 2011
>> New Revision: 1091573
>>
>> URL: http://svn.apache.org/viewvc?rev=1091573&view=rev
>> Log:
>> Followup to r1090784:
>>
>> * build.conf
>> (libsvn_ra.libs): Add libsvn_fs_util.
> Whoa. This doesn't feel right to me. Well, let me clarify, as I can see
> why r1090784 makes this necessary.
>
> It's r1090784 which looks severely problematic to me. The RA loader module
> is not permitted to access svn_fs_ functions directly! It may only call the
> vtable functions provided by the individual RA plugins. Of those plugins,
> ra_local is the only one permitted to call svn_fs_ functions.
>
> And now that I'm looking at the svn_fs_get_cache_config(), I'm feeling wrong
> about that, too. If this cache stuff is an FSFS-only thing, then it needs
> to live inside libsvn_fs_fs. libsvn_fs_util exists *only* for use by FS
> implementations (libsvn_fs_fs and libsvn_fs_base, today) which need to share
> utility functions.
>
> Sorry I didn't catch this stuff earlier (yes, guilty of "well, *someone*
> must be reviewing this code" syndrome), but I'm seeing what appears to be a
> series of screaming layering violations here.
The code in question has evolved over many months so it is
very possible that the name of svn_fs_get_cache_config()
and friends is no longer appropriate. The same goes for the
place that this has been implemented.
From a design perspective, I think it is perfectly to o.k. to
expose resource limits on the server UI level. The fact that FSFS
is currently the only part of the server that uses these settings
does not change, IMO, the fact that they are part of the UI.
Here is where a design issue with our RA layer concept comes
into play: "server" as in "repository content access provider"
can be almost any svn executable: apache/mod_dav_svn,
svnserve, svn, svnadmin, ...
Maybe we should simply move the function in question to libsvn_subr
and rename them properly.
Comments?
-- Stefan^2.