Re: svn commit: r1803639 - in /subversion/trunk: build/run_tests.py subversion/libsvn_fs_fs/fs.h subversion/libsvn_fs_fs/fs_fs.c subversion/libsvn_fs_fs/transaction.c subversion/tests/cmdline/svntest/main.py win-tests.py

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1803639 - in /subversion/trunk: build/run_tests.py subversion/libsvn_fs_fs/fs.h subversion/libsvn_fs_fs/fs_fs.c subversion/libsvn_fs_fs/transaction.c subversion/tests/cmdline/svntest/main.py win-tests.py

Daniel Shahaf-2
[hidden email] wrote on Tue, 01 Aug 2017 12:18 +0000:

> Author: kotkov
> Date: Tue Aug  1 12:18:23 2017
> New Revision: 1803639
>
> URL: http://svn.apache.org/viewvc?rev=1803639&view=rev
> Log:
> fsfs: Introduce new 'compression' config option.
>
> This option allows explicitly specifying the compression algorithm for
> format 8 repositories.  It deprecates the previously used 'compression-level'
> option.  The syntax of the new option is:
>
>   compression = none | lz4 | zlib | zlib-1 ... zlib-9

Thanks for implementing this, Evgeny.  One comment:

> +++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Tue Aug  1 12:18:23 2017
> @@ -947,23 +1052,25 @@ write_config(svn_fs_t *fs,
>  "# " CONFIG_OPTION_MAX_LINEAR_DELTIFICATION " = 16"                          NL
>  "###"                                                                        NL
>  "### After deltification, we compress the data to minimize on-disk size."    NL
> +"### This setting controls the compression algorithm, which will be used in" NL
> +"### future revisions.  It can be used to either disable compression or to"  NL
> +"### select between available algorithms (zlib, lz4).  zlib is a general-"   NL
> +"### purpose compression algorithm.  lz4 is a fast compression algorithm"    NL
> +"### which should be preferred for repositories with large and, possibly,"   NL
> +"### incompressible files.  Note that the compression ratio of lz4 is"       NL
> +"### usually lower than the one provided by zlib, but using it can"          NL
> +"### significantly speed up commits as well as reading the data."            NL
> +"### The syntax of this option is:"                                          NL
> +"###   " CONFIG_OPTION_COMPRESSION " = none | lz4 | zlib | zlib-1 ... zlib-9" NL
> +"### The default value is 'zlib', which is currently equivalent to 'zlib-5'." NL
> +"# " CONFIG_OPTION_COMPRESSION " = zlib"                                     NL
> +"###"                                                                        NL
> +"### DEPRECATED: The new '" CONFIG_OPTION_COMPRESSION "' option deprecates previously used" NL
> +"### '" CONFIG_OPTION_COMPRESSION_LEVEL "' option, which was used to configure zlib compression." NL
> +"### For compatibility with previous versions of Subversion, this option can"NL
> +"### still be used (and it will result in zlib compression with the"         NL
> +"### corresponding compression level)."                                      NL
> +"###   " CONFIG_OPTION_COMPRESSION_LEVEL " = 0 ... 9 (default is 5)"         NL

The documentation implies that CONFIG_OPTION_COMPRESSION can be used
regardless of the filesystem format, …

> @@ -683,6 +683,60 @@ verify_block_size(apr_int64_t block_size
> +static svn_error_t *
> +parse_compression_option(compression_type_t *compression_type_p,
> +                         int *compression_level_p,
> +                         const char *value)
> +{
> @@ -816,6 +859,68 @@ read_config(fs_fs_data_t *ffd,
>      {
>        ffd->pack_after_commit = FALSE;
>      }
> +
> +  /* Initialize compression settings in ffd. */
> +  if (ffd->format >= SVN_FS_FS__MIN_SVNDIFF2_FORMAT)
> +    {
> +      svn_config_get(config, &compression_val,
> +                     CONFIG_SECTION_DELTIFICATION,
> +                     CONFIG_OPTION_COMPRESSION, NULL);
> +      svn_config_get(config, &compression_level_val,
> +                     CONFIG_SECTION_DELTIFICATION,
> +                     CONFIG_OPTION_COMPRESSION_LEVEL, NULL);
> +    }
> +  else if (ffd->format >= SVN_FS_FS__MIN_DELTIFICATION_FORMAT)
> +    {
> +      SVN_ERR(svn_config_get_int64(config, &compression_level,
> +                                   CONFIG_SECTION_DELTIFICATION,
> +                                   CONFIG_OPTION_COMPRESSION_LEVEL,
> +                                   SVN_DELTA_COMPRESSION_LEVEL_DEFAULT));

… but the code only reads CONFIG_OPTION_COMPRESSION in f8 filesystems.

Given the docs as written, I would expect to be able to edit fsfs.conf
and replace 'compression-level = 4' with 'compression = zlib-4' without
doing an 'svnadmin upgrade'; so I think the SVN_FS_FS__MIN_DELTIFICATION_FORMAT
codepath should honour CONFIG_OPTION_COMPRESSION (and error out if it is
set to "lz4").

Cheers,

Daniel

> +    }
> +  else if (ffd->format >= SVN_FS_FS__MIN_SVNDIFF1_FORMAT)
> +    {
> +      ffd->delta_compression_type = compression_type_zlib;
> +      ffd->delta_compression_level = SVN_DELTA_COMPRESSION_LEVEL_DEFAULT;
> +    }
> +  else
> +    {
> +      ffd->delta_compression_type = compression_type_none;
> +      ffd->delta_compression_level = SVN_DELTA_COMPRESSION_LEVEL_NONE;
> +    }
> +
>  #ifdef SVN_DEBUG
>    SVN_ERR(svn_config_get_bool(config, &ffd->verify_before_commit,
>                                CONFIG_SECTION_DEBUG,
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1803639 - in /subversion/trunk: build/run_tests.py subversion/libsvn_fs_fs/fs.h subversion/libsvn_fs_fs/fs_fs.c subversion/libsvn_fs_fs/transaction.c subversion/tests/cmdline/svntest/main.py win-tests.py

Evgeny Kotkov
Daniel Shahaf <[hidden email]> writes:

> The documentation implies that CONFIG_OPTION_COMPRESSION can be used
> regardless of the filesystem format, …

> … but the code only reads CONFIG_OPTION_COMPRESSION in f8 filesystems.
>
> Given the docs as written, I would expect to be able to edit fsfs.conf
> and replace 'compression-level = 4' with 'compression = zlib-4' without
> doing an 'svnadmin upgrade'; so I think the SVN_FS_FS__MIN_DELTIFICATION_FORMAT
> codepath should honour CONFIG_OPTION_COMPRESSION (and error out if it is
> set to "lz4").

Unless I am missing something subtle, it would be an incompatible change.

(For example, then the same fsfs.conf file in format 7 repository with
"compression=zlib-4" would result in different behavior, depending on
whether the repository is opened with Subversion 1.9 or 1.10.)

That's the reason why the option is currently implemented only for format 8.

But in the meanwhile, I think that I see the point that the documentation
doesn't mention this.  Perhaps, we could say something along the following
lines to avoid the potential confusion?

[[[
### After deltification, we compress the data to minimize on-disk size.
### These settings control the compression algorithm, which will be used in
### future revisions.  It can be used to either disable compression or to
### select between available algorithms (zlib, lz4).  zlib is a general-
### purpose compression algorithm.  lz4 is a fast compression algorithm
### which should be preferred for repositories with large and, possibly,
### incompressible files.  Note that the compression ratio of lz4 is
### usually lower than the one provided by zlib, but using it can
### significantly speed up commits as well as reading the data.
###
### In format 8 repositories created by Subversion 1.10, compression can
### be configured using the following option:
###   compression = none | lz4 | zlib | zlib-1 ... zlib-9
### For new formats, 'compression' option DEPRECATES the previously used
### 'compression-level' option (see below).  For compatibility, this
### deprecated option can still be used, and that will result in zlib
### compression with the corresponding compression level.
### The default value is 'zlib', which is currently equivalent to 'zlib-5'.
# compression = zlib
###
### In format 4-7 repositories, only zlib compression algorithm is
### supported.  Its compression level can be configured with the
### following option:
###   compression-level = 0 ... 9 (default is 5)
# compression-level = 5
]]]


Regards,
Evgeny Kotkov
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1803639 - in /subversion/trunk: build/run_tests.py subversion/libsvn_fs_fs/fs.h subversion/libsvn_fs_fs/fs_fs.c subversion/libsvn_fs_fs/transaction.c subversion/tests/cmdline/svntest/main.py win-tests.py

Daniel Shahaf-2
Evgeny Kotkov wrote on Wed, Aug 02, 2017 at 15:25:33 +0300:

> Daniel Shahaf <[hidden email]> writes:
>
> > The documentation implies that CONFIG_OPTION_COMPRESSION can be used
> > regardless of the filesystem format, …
>
> > … but the code only reads CONFIG_OPTION_COMPRESSION in f8 filesystems.
> >
> > Given the docs as written, I would expect to be able to edit fsfs.conf
> > and replace 'compression-level = 4' with 'compression = zlib-4' without
> > doing an 'svnadmin upgrade'; so I think the SVN_FS_FS__MIN_DELTIFICATION_FORMAT
> > codepath should honour CONFIG_OPTION_COMPRESSION (and error out if it is
> > set to "lz4").
>
> Unless I am missing something subtle, it would be an incompatible change.
>
> (For example, then the same fsfs.conf file in format 7 repository with
> "compression=zlib-4" would result in different behavior, depending on
> whether the repository is opened with Subversion 1.9 or 1.10.)
>
> That's the reason why the option is currently implemented only for format 8.

Fair point.

> But in the meanwhile, I think that I see the point that the documentation
> doesn't mention this.  Perhaps, we could say something along the following
> lines to avoid the potential confusion?
>
> [[[
> ### In format 8 repositories created by Subversion 1.10, compression can
> ### be configured using the following option:
> ###   compression = none | lz4 | zlib | zlib-1 ... zlib-9
> ### For new formats, 'compression' option DEPRECATES the previously used
> ### 'compression-level' option (see below).
> ⋮
> ### In format 4-7 repositories, only zlib compression algorithm is
> ### supported.  Its compression level can be configured with the
> ### following option:
> ###   compression-level = 0 ... 9 (default is 5)

Hmm.  My first instinct would be to make the availability of the
'compression' knob coupled, not with the format number but with
SVN_VER_MINOR, so the rule would be "1.10 recognises the 'compression'
knob if set; 1.9 ignores it".

That _would_ cause the knob to be silently ignored on downgrades, but
the failure mode wouldn't be too bad (some performance loss; that's
expected when downgrading), and it would only affect people who edited
fsfs.conf from the default.  Moreover, we can even teach fsfs to warn
whenever it sees an unrecognised option in the config file, similar to
the cmdline client:

    % svn info --config-option=config:foo:bar=1
    svn: warning: apr_err=SVN_ERR_CL_ARG_PARSING_ERROR
    svn: warning: W205000: Ignoring unknown value 'foo'
    Path: .

On the other hand, recognising the knob only in f8+ would require
administrators to learn two different ways to do one thing: "In one kind
of repositories, you disable compression by doing X, and in another kind
of repositories, you disable compression by doing Y" (where the two
kinds are "≥f8" and "≤f7").  This fails PEP 20.

All in all, I think I'm leaning towards making the knob conditional on
SVN_VER_MINOR rather than ffd->format, but not strongly.  Let's say I'm
+0.5 to SVN_VER_MINOR.

Supposing the knob is coupled with the format number, should we issue
warnings about that?  For example, a warning in 1.10 about 'compression'
being set when opening a f7-or-older repository, or a warning when
'compression' is set and upgrading f7-or-older to f8-or-newer?  These
situations, too, would change the observed behaviour.

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1803639 - in /subversion/trunk: build/run_tests.py subversion/libsvn_fs_fs/fs.h subversion/libsvn_fs_fs/fs_fs.c subversion/libsvn_fs_fs/transaction.c subversion/tests/cmdline/svntest/main.py win-tests.py

Evgeny Kotkov
Daniel Shahaf <[hidden email]> writes:

> Hmm.  My first instinct would be to make the availability of the
> 'compression' knob coupled, not with the format number but with
> SVN_VER_MINOR, so the rule would be "1.10 recognises the 'compression'
> knob if set; 1.9 ignores it".
>
> That _would_ cause the knob to be silently ignored on downgrades, but
> the failure mode wouldn't be too bad (some performance loss; that's
> expected when downgrading), and it would only affect people who edited
> fsfs.conf from the default.

At this point, I am not too sure about why would we want to have this
different behavior for 'compression'.  We haven't been doing this for
other config options that are coupled with certain format numbers —
and I think there is a plenty of such options.

Also, that would mean we would be retroactively adding support for the
new option to formats that did not have it when they have been released.
I think that rather important property that we try to keep is that, when
working with older repository formats, new versions of Subversion write
the same data to the disk that would've been written by the old versions.
(Unless there's a complelling reason to change this, say, to fix a bug.)

Somewhat orthogonally:

What's a downgrade in this context?  As far as I recall, we don't have an
official way to downgrade a repository, and if it's the "create new repository
with --compatible-version, dump/load and replace the fsfs.conf with the one
you had in the newer repository", then I would say that falls a bit out of
scope.

A lot of existing options, e.g. the whole [deltification] or [io] sections,
are only available starting from the specific formats, so it would probably
be unwise for such users to assume that everything is going to (magically ;-)
work as before.

> On the other hand, recognising the knob only in f8+ would require
> administrators to learn two different ways to do one thing: "In one kind
> of repositories, you disable compression by doing X, and in another kind
> of repositories, you disable compression by doing Y" (where the two
> kinds are "≥f8" and "≤f7").  This fails PEP 20.

I think that it's the price of introducing the new explicit option that is
now used in favor of the old one.  But that has been sort of the point why
we implemented it — to be explicit about what get's written on the disk.

> All in all, I think I'm leaning towards making the knob conditional on
> SVN_VER_MINOR rather than ffd->format, but not strongly.  Let's say I'm
> +0.5 to SVN_VER_MINOR.
>
> Supposing the knob is coupled with the format number, should we issue
> warnings about that?  For example, a warning in 1.10 about 'compression'
> being set when opening a f7-or-older repository, or a warning when
> 'compression' is set and upgrading f7-or-older to f8-or-newer?  These
> situations, too, would change the observed behaviour.

That could be a possible improvement, but, on the other hand, I don't think
we have been doing this for existing options that can only be used starting
from certain formats.

Do you think that something is clearly broken in its current state?  Because
personally, I am quite happy with what we have now in trunk (assuming the
updated fsfs.conf docs).


Thanks,
Evgeny Kotkov
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1803639 - in /subversion/trunk: build/run_tests.py subversion/libsvn_fs_fs/fs.h subversion/libsvn_fs_fs/fs_fs.c subversion/libsvn_fs_fs/transaction.c subversion/tests/cmdline/svntest/main.py win-tests.py

Daniel Shahaf-2
Evgeny Kotkov wrote on Thu, Aug 03, 2017 at 14:34:21 +0300:

> Daniel Shahaf <[hidden email]> writes:
>
> > Hmm.  My first instinct would be to make the availability of the
> > 'compression' knob coupled, not with the format number but with
> > SVN_VER_MINOR, so the rule would be "1.10 recognises the 'compression'
> > knob if set; 1.9 ignores it".
> >
> > That _would_ cause the knob to be silently ignored on downgrades, but
> > the failure mode wouldn't be too bad (some performance loss; that's
> > expected when downgrading), and it would only affect people who edited
> > fsfs.conf from the default.
>
> At this point, I am not too sure about why would we want to have this
> different behavior for 'compression'.  We haven't been doing this for
> other config options that are coupled with certain format numbers —
> and I think there is a plenty of such options.

We have a mix of options that depend on SVN_VER_MINOR and on
ffd->format.

SVN_VER_MINOR:
  [memcached-servers]
  [caches]
  [debug]

ffd->format:
  [rep-sharing]
  [packed-revprops]
  [io]

both:
  [deltification] - requires ≥1.8 and ≥f4

> Also, that would mean we would be retroactively adding support for the
> new option to formats that did not have it when they have been released.
> I think that rather important property that we try to keep is that, when
> working with older repository formats, new versions of Subversion write
> the same data to the disk that would've been written by the old versions.
> (Unless there's a complelling reason to change this, say, to fix a bug.)

The worst that can happen is:

1. Admin uses 1.9
2. Admin installs 1.10
3. Admin changes a fsfs.conf setting by hand
4. Admin serves the repository using 1.9

and at that point, some reps would be written with the wrong compression.
Note that lz4 has nothing to do with this question, since neither 1.9
nor 1.10 will write svndiff2 data to a repository that 1.9 can serve,
since such a repository is necessarily ≤f7.  The only possibility of
confusion is if the admin sets 'compression' to one of 'none', 'zlib-1',
…, 'zlib-9' and then 1.9 will use another one of those ten.

As far as I can tell, that boils down to: if the admin changed the
defaults without RTFMing them, the admin will lose some performance.  

I think the "1.10's UI shouldn't depend on the format number" point
below trumps this concern.

> Somewhat orthogonally:
>
> What's a downgrade in this context?  As far as I recall, we don't have an
> official way to downgrade a repository, and if it's the "create new repository
> with --compatible-version, dump/load and replace the fsfs.conf with the one
> you had in the newer repository", then I would say that falls a bit out of
> scope.

A "downgrade" is "deinstall Subversion 1.10, install Subversion 1.9"
while keeping the repository at f7.

> A lot of existing options, e.g. the whole [deltification] or [io] sections,
> are only available starting from the specific formats, so it would probably
> be unwise for such users to assume that everything is going to (magically ;-)
> work as before.
>

Of course downgrading the libraries will bring changes.  That's true
even if the UI doesn't change.

> > On the other hand, recognising the knob only in f8+ would require
> > administrators to learn two different ways to do one thing: "In one kind
> > of repositories, you disable compression by doing X, and in another kind
> > of repositories, you disable compression by doing Y" (where the two
> > kinds are "≥f8" and "≤f7").  This fails PEP 20.
>
> I think that it's the price of introducing the new explicit option that is
> now used in favor of the old one.  But that has been sort of the point why
> we implemented it — to be explicit about what get's written on the disk.
>

I don't understand what you mean by "explicit".  I also don't understand
why it's an answer to "the way to disable compression in 1.10 is not the
same for all repositories, admins must learn two methods and remember to
check which one to use".  This design would be a textbook example of the
reason dev@'s consensus generally frowns upon new knobs.

> > All in all, I think I'm leaning towards making the knob conditional on
> > SVN_VER_MINOR rather than ffd->format, but not strongly.  Let's say I'm
> > +0.5 to SVN_VER_MINOR.
> >
> > Supposing the knob is coupled with the format number, should we issue
> > warnings about that?  For example, a warning in 1.10 about 'compression'
> > being set when opening a f7-or-older repository, or a warning when
> > 'compression' is set and upgrading f7-or-older to f8-or-newer?  These
> > situations, too, would change the observed behaviour.
>
> That could be a possible improvement, but, on the other hand, I don't think
> we have been doing this for existing options that can only be used starting
> from certain formats.
>
> Do you think that something is clearly broken in its current state?  Because
> personally, I am quite happy with what we have now in trunk (assuming the
> updated fsfs.conf docs).

Yes, I think the way to disable compression in 1.10 shouldn't depend on the
format number.

Sorry for the late reply.

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1803639 - in /subversion/trunk: build/run_tests.py subversion/libsvn_fs_fs/fs.h subversion/libsvn_fs_fs/fs_fs.c subversion/libsvn_fs_fs/transaction.c subversion/tests/cmdline/svntest/main.py win-tests.py

Evgeny Kotkov
Daniel Shahaf <[hidden email]> writes:

> We have a mix of options that depend on SVN_VER_MINOR and on
> ffd->format.
>
> SVN_VER_MINOR:
>   [memcached-servers]
>   [caches]
>   [debug]
>
> ffd->format:
>   [rep-sharing]
>   [packed-revprops]
>   [io]
>
> both:
>   [deltification] - requires ≥1.8 and ≥f4
I think that I have missed that the whole [deltification] section and even
the 'compression-level' option itself have been available starting from
certain minor versions, instead of just being new in some formats.

From this perspective, there's nothing special about the new 'compression'
option, and I should agree that making it available starting from 1.10 and
also applicable to older fs formats would be better.

While here, I think that we could also error out on a configuration with
both 'compression' and 'compression-level' set to limit the amount of
possible configurations.  (Currently, in such case the 'compression'
option silently overrides 'compression-level'.)

Perhaps, then, we could do all this as in the attached patch.


Thanks,
Evgeny Kotkov

fsfs-compression-opt-fixup-v1.patch.txt (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1803639 - in /subversion/trunk: build/run_tests.py subversion/libsvn_fs_fs/fs.h subversion/libsvn_fs_fs/fs_fs.c subversion/libsvn_fs_fs/transaction.c subversion/tests/cmdline/svntest/main.py win-tests.py

Daniel Shahaf-2
Evgeny Kotkov wrote on Tue, 08 Aug 2017 17:05 +0300:
> Perhaps, then, we could do all this as in the attached patch.

+1, and thanks for the productive discussion :-).
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1803639 - in /subversion/trunk: build/run_tests.py subversion/libsvn_fs_fs/fs.h subversion/libsvn_fs_fs/fs_fs.c subversion/libsvn_fs_fs/transaction.c subversion/tests/cmdline/svntest/main.py win-tests.py

Evgeny Kotkov
Daniel Shahaf <[hidden email]> writes:

>> Perhaps, then, we could do all this as in the attached patch.
>
> +1, and thanks for the productive discussion :-).

Thanks, committed in r1804646, r1804647 and r1804648 with a slightly
tweaked error message for the case when LZ4 is not supported by
the filesystem format:

+              return svn_error_create(SVN_ERR_UNSUPPORTED_FEATURE, NULL,
+                                      _("Compression type 'lz4' requires "
+                                        "filesystem format 8 or higher"));


Regards,
Evgeny Kotkov
Loading...