bzr has some hardcoded constants that could be configurable

[Vincent Ladeuil]

Many parts of bzrlib declare constants and there is no way for the user to look at or modify them.

Turning them into options without a significant performance impact will address that.

[Martin Pool (mbp)]

Well, there is a way, which is to look at the source code. :-)

If this bug is really saying that there are some things that are hardcoded but that people would like to change, I can in principle agree, but it's going to be a lot more compelling if there are complaints about specific cases.

[Martin Pool (mbp)]

I think this is only nice-to-have (almost incomplete) in the abstract, but if there are more specific cases that need to be fixed let's don them.

[Vincent Ladeuil (vila)]

We have may constants that have been established as being the "best" value based on experiments on a finite set of setups (branch history, number of files, number of directories, whatever). This best value is therefore not the best one for some setups.

Requiring the user to look (or modify) the source code forbids experimentation for values best suited for a particular setup. I.e. even if bzr can perform better in some cases, nobody can try better values.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 8/24/2011 8:55 AM, Vincent Ladeuil wrote:
> We have may constants that have been established as being the
> "best" value based on experiments on a finite set of setups (branch
> history, number of files, number of directories, whatever). This
> best value is therefore not the best one for some setups.
>
> Requiring the user to look (or modify) the source code forbids
> experimentation for values best suited for a particular setup. I.e.
> even if bzr can perform better in some cases, nobody can try better
> values.
>

I think Martin's point is that "yes, this is probably true, however
the bug doesn't actually motivate us to do anything". Without concrete
things to be done.

So if you want to change this to:

 bzrlib.Foo.Bar._baz

Is a hard-coded constant that could be better served as a config item.

Then it is something that we can actually *do* and close the bug.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5VHRYACgkQJdeBCYSNAAMX1wCgsc7Nn2i9t5R7bLMvT2OxjSiK
Es8An0oDavm95co3i69So0qT7xA9pBow
=OI0C
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

I've discussed it with Martin, the disagreement was two-fold:

- he felt the bug was too vague,

- I felt setting the importance to low wasn't the right way to
  convey that the priority is low (which I agree with, I don't
  intend to fix it until the migration to the new design is
  completed, including addressing the performance issue)

I answered his concern about vagueness by explaining that I think
there are ~10 at worst 20 such constants and that I don't know
them all.

Here are the ones I can find after a quick grepping:

bzrlib.workingtree.worth_saving_limit
bzrlib.groupcompress.BATCH_SIZE
bzrlib.groupcompress.GroupCompressVersionedFiles._DEFAULT_MAX_BYTES_TO_INDEX
bzrlib.lockdir._DEFAULT_TIMEOUT_SECONDS
bzrlib.lockdir.__DEFAULT_POLL_SECONDS
bzrlib.add._DEFAULT_MAX_FILE_SIZE

I'm sure you know more, feel free to add them (we have time to
list them before this bug is fixed anyway).

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/24/2011 07:09 PM, Vincent Ladeuil wrote:
> I've discussed it with Martin, the disagreement was two-fold:
>
> - he felt the bug was too vague,
>
> - I felt setting the importance to low wasn't the right way to
> convey that the priority is low (which I agree with, I don't
> intend to fix it until the migration to the new design is
> completed, including addressing the performance issue)
>
> I answered his concern about vagueness by explaining that I think
> there are ~10 at worst 20 such constants and that I don't know
> them all.
>
> Here are the ones I can find after a quick grepping:
>
> bzrlib.workingtree.worth_saving_limit

^- This is configurable. See bzrlib.workingtree_4._worth_saving_limit

> bzrlib.groupcompress.BATCH_SIZE
> bzrlib.groupcompress.GroupCompressVersionedFiles._DEFAULT_MAX_BYTES_TO_INDEX

^- Similarly:
 bzrlib.groupcompress._LazyGroupContentManager._get_compressor_settings

> bzrlib.lockdir._DEFAULT_TIMEOUT_SECONDS
> bzrlib.lockdir.__DEFAULT_POLL_SECONDS
> bzrlib.add._DEFAULT_MAX_FILE_SIZE

Similarly bzrlib.add.AddWithSkipLargeAction.skip_file

>
> I'm sure you know more, feel free to add them (we have time to
> list them before this bug is fixed anyway).
>

Most are already configurable when we felt it would be useful. We just
didn't have the new syntax to be able to do so.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5WVQ8ACgkQJdeBCYSNAAMCxQCeLjf68xWHJ4TBTW6mF38v29uN
OykAoMu75ZGuXai9902sX0ye1VxbAzlm
=y7cO
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

@jam: I don't understand your comment, what do you mean with 'similarly' ?

Except for the first option which I migrated myself, none of the others are configurable as in 'associated with a config option'.

The intent of this bug is to give an access to such options without writing any python code.

> We just didn't have the new syntax to be able to do so.

Doh, yeah, that's why I don't suggest fixing this bug until there is an easy way.

[John A Meinel (jameinel)]

bzrlib.groupcompress.GroupCompressVersionedFiles._DEFAULT_MAX_BYTES_TO_INDEX
Is the *default*, but the value that is used at runtime is:

    def _get_compressor_settings(self):
        if self._max_bytes_to_index is None:
            # TODO: VersionedFiles don't know about their containing
            # repository, so they don't have much of an idea about their
            # location. So for now, this is only a global option.
            c = config.GlobalConfig()
            val = c.get_user_option('bzr.groupcompress.max_bytes_to_index')
            if val is not None:
                try:
                    val = int(val)
                except ValueError, e:
                    trace.warning('Value for '
                                  '"bzr.groupcompress.max_bytes_to_index"'
                                  ' %r is not an integer'
                                  % (val,))
                    val = None
            if val is None:
                val = self._DEFAULT_MAX_BYTES_TO_INDEX
            self._max_bytes_to_index = val

So *if* the user sets "bzr.groupcompress.max_bytes_to_index" in bazaar.conf, it supersedes the DEFAULT value. (I don't think we need to add a configuration to set the DEFAULT on top of being able to set the actual value, do you?)

"Similarly" _DEFAULT_MAX_FILE_SIZE:
    _DEFAULT_MAX_FILE_SIZE = 20000000
    _optionName = 'add.maximum_file_size'
    _maxSize = None

    def skip_file(self, tree, path, kind, stat_value = None):
        if kind != 'file':
            return False
        if self._maxSize is None:
            config = tree.branch.get_config()
            self._maxSize = config.get_user_option_as_int_from_SI(
                self._optionName,
                self._DEFAULT_MAX_FILE_SIZE)
...

So again the *DEFAULT* is a constant, but the actual value used is configurable with "add.maximum_file_size".

Yes, these could be rewritten to use the new config syntax. However, they are *already* configurable. So I don't understand whether this bug applies or not.

I don't think the locking stuff needs to be configurable. We could, but I think you're far of in diminishing returns at that point. (We've had the same value for ~3-5 years now.)

[Vincent Ladeuil (vila)]

>>>>> "]" == John A Meinel writes:

<snip/>

    ]> So *if* the user sets "bzr.groupcompress.max_bytes_to_index" in
    ]> bazaar.conf, it supersedes the DEFAULT value. (I don't think we need to
    ]> add a configuration to set the DEFAULT on top of being able to set the
    ]> actual value, do you?)

Of course not, this just need to me migrated and is not the kind of
constant I'm after here, but thanks for the precisions, as I said, this
was a quick grepping.

<snip/>

    ]> So again the *DEFAULT* is a constant, but the actual value used is
    ]> configurable with "add.maximum_file_size".

Same remark.

    ]> Yes, these could

should and will before *this* bug can be fixed.

    ]> be rewritten to use the new config syntax. However, they are
    ]> *already* configurable. So I don't understand whether this bug
    ]> applies or not.

Not to these specific ones then. That why I asked for others, if they
aren't any, there is nothing left to fix by this bug.

    ]> I don't think the locking stuff needs to be configurable. We
    ]> could, but I think you're far of in diminishing returns at that
    ]> point. (We've had the same value for ~3-5 years now.)

Well, consider the issue we encountered a couple of times with mounted
file systems on windows where we couldn't get enough data from the
user(s) able to reproduce the issue. IIRC, you and Martin gives quite
precise guidance about which code should be changed to acquire the
data. Despite your efforts, we're still waiting for data.

My point here is that if the infrastructure is in place we just have to
ask for a command to be executed, no code to write, no code to install.

Is there something wrong with that ?

I don't expect all such cases to be covered, but everywhere we decided
to define a constant, we already considered that some other value(s)
*may* potentially be useful (right ?), this bug aims at making them
reachable by a simple command. Nothing more.

Hence my previous question: where are the others ? If there is none
left, fine, let's close the bug, we can even close it now and re-open it
if needed.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> ]> I don't think the locking stuff needs to be configurable. We ]>
> could, but I think you're far of in diminishing returns at that ]>
> point. (We've had the same value for ~3-5 years now.)
>
> Well, consider the issue we encountered a couple of times with
> mounted file systems on windows where we couldn't get enough data
> from the user(s) able to reproduce the issue. IIRC, you and Martin
> gives quite precise guidance about which code should be changed to
> acquire the data. Despite your efforts, we're still waiting for
> data.

Actually, not related at all. The failures on Windows are because
"os.rename(src, target); open(target) # failing". Nothing configurable
that could have been done, because it was an unexpected case.

If we add a "try again after X milliseconds", yes, that could be
configurable.

John
=:->

>
> My point here is that if the infrastructure is in place we just
> have to ask for a command to be executed, no code to write, no code
> to install.
>
> Is there something wrong with that ?
>
> I don't expect all such cases to be covered, but everywhere we
> decided to define a constant, we already considered that some other
> value(s) *may* potentially be useful (right ?), this bug aims at
> making them reachable by a simple command. Nothing more.
>
> Hence my previous question: where are the others ? If there is
> none left, fine, let's close the bug, we can even close it now and
> re-open it if needed.
>

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5Y44AACgkQJdeBCYSNAAN69QCffPmtwCdQAlRCmYxwzoss2L4h
hN0AoKZ7/2H2j1Yum7eFZYfGE5KwbrIz
=uUkT
-----END PGP SIGNATURE-----