You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ko...@apache.org on 2017/08/01 12:18:23 UTC
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
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
See the related discussion in:
https://lists.apache.org/thread.html/40650a309d8ff041adbb62e8ffe19cc3990b9098a032db932fabd170@%3Cdev.subversion.apache.org%3E
* subversion/libsvn_fs_fs/fs.h
(CONFIG_OPTION_COMPRESSION): New.
(compression_type_t): New.
(fs_fs_data_t): Add field to store the delta compression type.
* subversion/libsvn_fs_fs/fs.h
(write_config): Revamp the section describing delta compression.
(parse_compression_option): New helper function.
(read_config): Parse the new 'compression' option when working with
newer formats, with a possible fall back to 'compression-level' in
case it's specified explicitly. In order to always have appropriate
and usable compression settings in ffd, move the part of the code that
disables compression when only svndiff0 is supported from ...
* subversion/libsvn_fs_fs/transaction.c
(txdelta_to_svndiff): ...this function. Adjust this function to select
the appropriate svndiff version, depending on the options.
* win-tests.py
(): Rename 'fsfs_compression_level' to 'fsfs_compression'.
(_usage_exit): Adjust usage text.
* build/run_tests.py
(): Update usage comment.
(_init_py_tests): Pass the option as string, not int.
(create_parser): Parse the option as string, not int.
* subversion/tests/cmdline/svntest/main.py
(parse_options): Only allow using the --fsfs-compression option with
--server-minor-version >= 10.
(_create_parser): Parse the option as string, not int.
(run_one): Pass the option as string, not int.
(_post_create_repos): Update the code that adjusts fsfs.conf.
Modified:
subversion/trunk/build/run_tests.py
subversion/trunk/subversion/libsvn_fs_fs/fs.h
subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
subversion/trunk/subversion/libsvn_fs_fs/transaction.c
subversion/trunk/subversion/tests/cmdline/svntest/main.py
subversion/trunk/win-tests.py
Modified: subversion/trunk/build/run_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/build/run_tests.py?rev=1803639&r1=1803638&r2=1803639&view=diff
==============================================================================
--- subversion/trunk/build/run_tests.py (original)
+++ subversion/trunk/build/run_tests.py Tue Aug 1 12:18:23 2017
@@ -33,7 +33,7 @@
[--httpd-version=<version>] [--httpd-whitelist=<version>]
[--config-file=<file>] [--ssl-cert=<file>]
[--exclusive-wc-locks] [--memcached-server=<url:port>]
- [--fsfs-compression=<n>]
+ [--fsfs-compression=<type>]
<abs_srcdir> <abs_builddir>
<prog ...>
@@ -276,8 +276,8 @@ class TestHarness:
cmdline.append('--exclusive-wc-locks')
if self.opts.memcached_server is not None:
cmdline.append('--memcached-server=%s' % self.opts.memcached_server)
- if self.opts.fsfs_compression_level is not None:
- cmdline.append('--fsfs-compression=%d' % self.opts.fsfs_compression_level)
+ if self.opts.fsfs_compression is not None:
+ cmdline.append('--fsfs-compression=%s' % self.opts.fsfs_compression)
self.py_test_cmdline = cmdline
@@ -1026,9 +1026,8 @@ def create_parser():
help='Use sqlite exclusive locking for working copies')
parser.add_option('--memcached-server', action='store',
help='Use memcached server at specified URL (FSFS only)')
- parser.add_option('--fsfs-compression', action='store', type='int',
- dest="fsfs_compression_level",
- help='Set compression level (for fsfs)')
+ parser.add_option('--fsfs-compression', action='store', type='str',
+ help='Set compression type (for fsfs)')
parser.set_defaults(set_log_level=None)
return parser
Modified: subversion/trunk/subversion/libsvn_fs_fs/fs.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs.h?rev=1803639&r1=1803638&r2=1803639&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/fs.h (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/fs.h Tue Aug 1 12:18:23 2017
@@ -118,6 +118,7 @@ extern "C" {
#define CONFIG_SECTION_DEBUG "debug"
#define CONFIG_OPTION_PACK_AFTER_COMMIT "pack-after-commit"
#define CONFIG_OPTION_VERIFY_BEFORE_COMMIT "verify-before-commit"
+#define CONFIG_OPTION_COMPRESSION "compression"
/* The format number of this filesystem.
This is independent of the repository format number, and
@@ -298,6 +299,13 @@ typedef struct window_cache_key_t
apr_uint64_t item_index;
} window_cache_key_t;
+typedef enum compression_type_t
+{
+ compression_type_none,
+ compression_type_zlib,
+ compression_type_lz4
+} compression_type_t;
+
/* Private (non-shared) FSFS-specific data for each svn_fs_t object.
Any caches in here may be NULL. */
typedef struct fs_fs_data_t
@@ -473,7 +481,10 @@ typedef struct fs_fs_data_t
* deltification history after which skip deltas will be used. */
apr_int64_t max_linear_deltification;
- /* Compression level to use with txdelta storage format in new revs. */
+ /* Compression type to use with txdelta storage format in new revs. */
+ compression_type_t delta_compression_type;
+
+ /* Compression level (currently, only used with compression_type_zlib). */
int delta_compression_level;
/* Pack after every commit. */
Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1803639&r1=1803638&r2=1803639&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Tue Aug 1 12:18:23 2017
@@ -683,6 +683,60 @@ verify_block_size(apr_int64_t block_size
return SVN_NO_ERROR;
}
+static svn_error_t *
+parse_compression_option(compression_type_t *compression_type_p,
+ int *compression_level_p,
+ const char *value)
+{
+ compression_type_t type;
+ int level;
+ svn_boolean_t is_valid = TRUE;
+
+ /* compression = none | lz4 | zlib | zlib-1 ... zlib-9 */
+ if (strcmp(value, "none") == 0)
+ {
+ type = compression_type_none;
+ level = SVN_DELTA_COMPRESSION_LEVEL_NONE;
+ }
+ else if (strcmp(value, "lz4") == 0)
+ {
+ type = compression_type_lz4;
+ level = SVN_DELTA_COMPRESSION_LEVEL_DEFAULT;
+ }
+ else if (strncmp(value, "zlib", 4) == 0)
+ {
+ const char *p = value + 4;
+
+ type = compression_type_zlib;
+ if (*p == 0)
+ {
+ level = SVN_DELTA_COMPRESSION_LEVEL_DEFAULT;
+ }
+ else if (*p == '-')
+ {
+ p++;
+ SVN_ERR(svn_cstring_atoi(&level, p));
+ if (level < 1 || level > 9)
+ is_valid = FALSE;
+ }
+ else
+ is_valid = FALSE;
+ }
+ else
+ {
+ is_valid = FALSE;
+ }
+
+ if (!is_valid)
+ return svn_error_createf(SVN_ERR_BAD_CONFIG_VALUE, NULL,
+ _("Invalid 'compression' value '%s' in the config"),
+ value);
+
+ *compression_type_p = type;
+ *compression_level_p = level;
+ return SVN_NO_ERROR;
+}
+
/* Read the configuration information of the file system at FS_PATH
* and set the respective values in FFD. Use pools as usual.
*/
@@ -709,8 +763,6 @@ read_config(fs_fs_data_t *ffd,
/* Initialize deltification settings in ffd. */
if (ffd->format >= SVN_FS_FS__MIN_DELTIFICATION_FORMAT)
{
- apr_int64_t compression_level;
-
SVN_ERR(svn_config_get_bool(config, &ffd->deltify_directories,
CONFIG_SECTION_DELTIFICATION,
CONFIG_OPTION_ENABLE_DIR_DELTIFICATION,
@@ -727,14 +779,6 @@ read_config(fs_fs_data_t *ffd,
CONFIG_SECTION_DELTIFICATION,
CONFIG_OPTION_MAX_LINEAR_DELTIFICATION,
SVN_FS_FS_MAX_LINEAR_DELTIFICATION));
-
- SVN_ERR(svn_config_get_int64(config, &compression_level,
- CONFIG_SECTION_DELTIFICATION,
- CONFIG_OPTION_COMPRESSION_LEVEL,
- SVN_DELTA_COMPRESSION_LEVEL_DEFAULT));
- ffd->delta_compression_level
- = (int)MIN(MAX(SVN_DELTA_COMPRESSION_LEVEL_NONE, compression_level),
- SVN_DELTA_COMPRESSION_LEVEL_MAX);
}
else
{
@@ -742,7 +786,6 @@ read_config(fs_fs_data_t *ffd,
ffd->deltify_properties = FALSE;
ffd->max_deltification_walk = SVN_FS_FS_MAX_DELTIFICATION_WALK;
ffd->max_linear_deltification = SVN_FS_FS_MAX_LINEAR_DELTIFICATION;
- ffd->delta_compression_level = SVN_DELTA_COMPRESSION_LEVEL_DEFAULT;
}
/* Initialize revprop packing settings in ffd. */
@@ -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)
+ {
+ const char *compression_val;
+ const char *compression_level_val;
+
+ 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);
+ if (compression_val)
+ {
+ /* 'compression' option overrides deprecated 'compression-level'. */
+ SVN_ERR(parse_compression_option(&ffd->delta_compression_type,
+ &ffd->delta_compression_level,
+ compression_val));
+ }
+ else if (compression_level_val)
+ {
+ /* Handle the deprecated 'compression-level' option. */
+ ffd->delta_compression_type = compression_type_zlib;
+ SVN_ERR(svn_cstring_atoi(&ffd->delta_compression_level,
+ compression_level_val));
+ ffd->delta_compression_level =
+ MIN(MAX(SVN_DELTA_COMPRESSION_LEVEL_NONE,
+ ffd->delta_compression_level),
+ SVN_DELTA_COMPRESSION_LEVEL_MAX);
+ }
+ else
+ {
+ /* Nothing specified explicitly, use default settings. */
+ ffd->delta_compression_type = compression_type_zlib;
+ ffd->delta_compression_level = SVN_DELTA_COMPRESSION_LEVEL_DEFAULT;
+ }
+ }
+ else if (ffd->format >= SVN_FS_FS__MIN_DELTIFICATION_FORMAT)
+ {
+ apr_int64_t compression_level;
+
+ SVN_ERR(svn_config_get_int64(config, &compression_level,
+ CONFIG_SECTION_DELTIFICATION,
+ CONFIG_OPTION_COMPRESSION_LEVEL,
+ SVN_DELTA_COMPRESSION_LEVEL_DEFAULT));
+ ffd->delta_compression_type = compression_type_zlib;
+ ffd->delta_compression_level =
+ (int)MIN(MAX(SVN_DELTA_COMPRESSION_LEVEL_NONE, compression_level),
+ SVN_DELTA_COMPRESSION_LEVEL_MAX);
+ }
+ 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,
@@ -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 settings control the compression level for this process." NL
-"### Revisions with highly compressible data in them may shrink in size" NL
-"### if the setting is increased but may take much longer to commit." NL
-"### The time taken to uncompress that data again is widely independent" NL
-"### of the compression level. Compression will be ineffective if the" NL
-"### incoming content is already highly compressed. In that case," NL
-"### disabling the compression entirely or using the special value 1" NL
-"### (see below) will speed up commits as well as reading the data." NL
-"### Repositories with many small compressible files (source code) but" NL
-"### also a high percentage of large incompressible ones (artwork) may" NL
-"### benefit from compression levels lowered." NL
-"### Valid values are 0 to 9 with 9 providing the highest compression" NL
-"### ratio and 0 disabling it altogether. Using 1 as the level enables" NL
-"### LZ4 compression that provides a decent compression ratio, but" NL
-"### performs better with large or incompressible files." NL
-"### The default value is 5." NL
-"# " CONFIG_OPTION_COMPRESSION_LEVEL " = 5" 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
"" NL
"[" CONFIG_SECTION_PACKED_REVPROPS "]" NL
"### This parameter controls the size (in kBytes) of packed revprop files." NL
Modified: subversion/trunk/subversion/libsvn_fs_fs/transaction.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/transaction.c?rev=1803639&r1=1803638&r2=1803639&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/transaction.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/transaction.c Tue Aug 1 12:18:23 2017
@@ -2168,27 +2168,24 @@ txdelta_to_svndiff(svn_txdelta_window_ha
{
fs_fs_data_t *ffd = fs->fsap_data;
int svndiff_version;
- int svndiff_compression_level;
- if (ffd->delta_compression_level == 1 &&
- ffd->format >= SVN_FS_FS__MIN_SVNDIFF2_FORMAT)
+ if (ffd->delta_compression_type == compression_type_lz4)
{
+ SVN_ERR_ASSERT_NO_RETURN(ffd->format >= SVN_FS_FS__MIN_SVNDIFF2_FORMAT);
svndiff_version = 2;
- svndiff_compression_level = SVN_DELTA_COMPRESSION_LEVEL_DEFAULT;
}
- else if (ffd->format >= SVN_FS_FS__MIN_SVNDIFF1_FORMAT)
+ else if (ffd->delta_compression_type == compression_type_zlib)
{
+ SVN_ERR_ASSERT_NO_RETURN(ffd->format >= SVN_FS_FS__MIN_SVNDIFF1_FORMAT);
svndiff_version = 1;
- svndiff_compression_level = ffd->delta_compression_level;
}
else
{
svndiff_version = 0;
- svndiff_compression_level = SVN_DELTA_COMPRESSION_LEVEL_NONE;
}
svn_txdelta_to_svndiff3(handler, handler_baton, output, svndiff_version,
- svndiff_compression_level, pool);
+ ffd->delta_compression_level, pool);
}
/* Get a rep_write_baton and store it in *WB_P for the representation
Modified: subversion/trunk/subversion/tests/cmdline/svntest/main.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/svntest/main.py?rev=1803639&r1=1803638&r2=1803639&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/svntest/main.py (original)
+++ subversion/trunk/subversion/tests/cmdline/svntest/main.py Tue Aug 1 12:18:23 2017
@@ -1043,14 +1043,14 @@ def _post_create_repos(path, minor_versi
shutil.copy(options.config_file, confpath)
if options.memcached_server is not None or \
- options.fsfs_compression_level is not None and \
+ options.fsfs_compression is not None and \
os.path.exists(confpath):
with open(confpath, 'r') as conffile:
newlines = []
for line in conffile.readlines():
- if line.startswith('# compression-level ') and \
- options.fsfs_compression_level is not None:
- line = 'compression-level = %d\n' % options.fsfs_compression_level
+ if line.startswith('# compression ') and \
+ options.fsfs_compression is not None:
+ line = 'compression = %s\n' % options.fsfs_compression
newlines += line
if options.memcached_server is not None and \
line == '[memcached-servers]\n':
@@ -1737,8 +1737,8 @@ class TestSpawningThread(threading.Threa
args.append('--fsfs-version=' + str(options.fsfs_version))
if options.dump_load_cross_check:
args.append('--dump-load-cross-check')
- if options.fsfs_compression_level:
- args.append('--fsfs-compression=' + str(options.fsfs_compression_level))
+ if options.fsfs_compression:
+ args.append('--fsfs-compression=' + options.fsfs_compression)
result, stdout_lines, stderr_lines = spawn_process(command, 0, False, None,
*args)
@@ -2151,9 +2151,8 @@ def _create_parser(usage=None):
help='Use sqlite exclusive locking for working copies')
parser.add_option('--memcached-server', action='store',
help='Use memcached server at specified URL (FSFS only)')
- parser.add_option('--fsfs-compression', action='store', type='int',
- dest="fsfs_compression_level",
- help='Set compression level (for fsfs)')
+ parser.add_option('--fsfs-compression', action='store', type='str',
+ help='Set compression type (for fsfs)')
# most of the defaults are None, but some are other values, set them here
parser.set_defaults(
@@ -2203,9 +2202,9 @@ def parse_options(arglist=sys.argv[1:],
if options.fsfs_packing and not options.fsfs_sharding:
parser.error("--fsfs-packing requires --fsfs-sharding")
- if options.fsfs_compression_level is not None and\
- options.fsfs_compression_level not in range(0, 10):
- parser.error("--fsfs-compression must be between 0 and 9")
+ if options.fsfs_compression is not None and \
+ options.server_minor_version < 10:
+ parser.error("--fsfs-compression requires --server-minor-version=10")
if options.server_minor_version not in range(3, SVN_VER_MINOR+1):
parser.error("test harness only supports server minor versions 3-%d"
Modified: subversion/trunk/win-tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/win-tests.py?rev=1803639&r1=1803638&r2=1803639&view=diff
==============================================================================
--- subversion/trunk/win-tests.py (original)
+++ subversion/trunk/win-tests.py Tue Aug 1 12:18:23 2017
@@ -113,7 +113,7 @@ def _usage_exit():
print(" --config-file : Configuration file for tests")
print(" --fsfs-sharding : Specify shard size (for fsfs)")
print(" --fsfs-packing : Run 'svnadmin pack' automatically")
- print(" --fsfs-compression=VAL : Set compression level to VAL (for fsfs)")
+ print(" --fsfs-compression=VAL : Set compression type to VAL (for fsfs)")
print(" -q, --quiet : Deprecated; this is the default.")
print(" Use --set-log-level instead.")
@@ -191,7 +191,7 @@ memcached_server = None
memcached_dir = None
skip_c_tests = None
dump_load_cross_check = None
-fsfs_compression_level = None
+fsfs_compression = None
for opt, val in opts:
if opt in ('-h', '--help'):
@@ -287,7 +287,7 @@ for opt, val in opts:
memcached_dir = val
run_memcached = 1
elif opt == '--fsfs-compression':
- fsfs_compression_level = int(val)
+ fsfs_compression = val
# Calculate the source and test directory names
abs_srcdir = os.path.abspath("")
@@ -1113,7 +1113,7 @@ if not test_javahl and not test_swig:
opts.memcached_server = memcached_server
opts.skip_c_tests = skip_c_tests
opts.dump_load_cross_check = dump_load_cross_check
- opts.fsfs_compression_level = fsfs_compression_level
+ opts.fsfs_compression = fsfs_compression
th = run_tests.TestHarness(abs_srcdir, abs_builddir,
log_file, fail_log_file, opts)
old_cwd = os.getcwd()
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
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Daniel Shahaf <d....@daniel.shahaf.name> 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
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
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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 :-).
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
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Daniel Shahaf <d....@daniel.shahaf.name> 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
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
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Daniel Shahaf <d....@daniel.shahaf.name> 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
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
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Evgeny Kotkov wrote on Thu, Aug 03, 2017 at 14:34:21 +0300:
> Daniel Shahaf <d....@daniel.shahaf.name> 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
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
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Evgeny Kotkov wrote on Thu, Aug 03, 2017 at 14:34:21 +0300:
> Daniel Shahaf <d....@daniel.shahaf.name> 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
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
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Daniel Shahaf <d....@daniel.shahaf.name> 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
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
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Daniel Shahaf <d....@daniel.shahaf.name> 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
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
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Evgeny Kotkov wrote on Wed, Aug 02, 2017 at 15:25:33 +0300:
> Daniel Shahaf <d....@daniel.shahaf.name> 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
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
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Evgeny Kotkov wrote on Wed, Aug 02, 2017 at 15:25:33 +0300:
> Daniel Shahaf <d....@daniel.shahaf.name> 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
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
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Daniel Shahaf <d....@daniel.shahaf.name> 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
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
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Daniel Shahaf <d....@daniel.shahaf.name> 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
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
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
kotkov@apache.org 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,
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
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
kotkov@apache.org 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,