Code review comment for lp:~axwalk/juju-core/lp1235717-refactor-simplestreams-writemetadata
Revision history for this message
| Andrew Wilkins (axwalk) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
0e1f616
(Get the code!)
On 2013/10/10 02:26:45, axw wrote:
> On 2013/10/09 16:29:42, rog wrote:
> > LGTM with some thoughts below.
> Thanks. I'll change the newMetadata/
but I think
> I'll leave the others to a followup.
> > For the future, I think that the "resolve" mode is unnecessary and
should go -
> > if we have items in the target bucket that we don't have hashes for,
we should
> > just ignore them. This will actually work fine in practice, I think,
even if
> we
> > abort a SyncTools half way through, because when we retry the
SyncTools, any
> > items in the target bucket that we didn't get around to adding to
the target
> > bucket simplestreams data should have metadata in the source
bucket's
> > simplestreams data.
> >
> > In general, I think retro-discovering hash data by trying to find
out what the
> > data currently is is a flawed idea - the hash of some tools should
be
> generated
> > on first upload and preserved forever after.
> I'd like to say I agree, however I know the requirements here are a
bit subtle.
> I think it'd be good to talk with wallyworld at SFO to nut it out.
I discussed this with wallyworld on IRC, and he agreed that it should go
in 1.18.
After 1.16, all tools in storage should have corresponding metadata with
size and hash.
> >
https:/
> > File environs/
> >
> >
https:/
> > environs/
> oldMetadata
> > []*ToolsMetadata) []*ToolsMetadata {
> > On 2013/10/09 14:28:25, axw wrote:
> > > On 2013/10/09 12:08:57, rog wrote:
> > > > I'm not sure that "new" and "old" are appropriate prefixes here
- they
> seem
> > to
> > > > imply that the arguments are treated differently, but isn't
MergeMetadata
> > > > commutative?
> > > >
> > > > Personally I'd use something like m0 and m1, but go with your
own thoughts
> > for
> > > > naming.
> > >
> > > It's not commutative, per the last sentence of the doc comment:
> > > "If both entries have information, prefer the entry from
newMetadata".
> > >
> > > The idea behind that is that if both have size&hash, then the one
from
> > > "newMetadata" is considered newer. That's to cater for copying
over the top
> of
> > > existing tools (say, a dev build).
> >
> > Hmm, that *should* never happen - a version should always correspond
only to
> one
> > particular tarball. I think if we do get a clash like this, then we
have a
> > problem that I suspect our current logic won't cope with.
> >
> > Perhaps this function should check for inconsistencies and return an
error if
> it
> > finds one.
> Yeah, that sounds sensible. The tools won't be copied over if they are
already
> in the target, so the "newMetadata" entry won't have a size/hash. So
this code
> is only handling a theoretical case, anyway. I'll make this change and
> repropose.
> >
https:/
> > environs/
> > On 2013/10/09 14:28:25, axw wrote:
> > > On 2013/10/09 12:08:57, rog wrote:
> > > > I know we do this elsewhere, but I've come to dislike
> > > > this idiom - it's an unsatisfactory halfway house between
> > > > named constants and bools. There's still code that
> > > > uses the bool semantics, which means that these names aren't
> > > > really helping much because you have to know their values
> > > > anyway.
> > > >
> > > > I'd prefer either to use int and iota or to stick with the bool
> > > > and lose the named constants.
> > >
> > > Consider a bool as (conceptually) a 1-bit unsigned int; then
what's the
> > > difference between that and int with constant named values? The
only
> > difference
> > > I see is that the bool is range limited to [0, 1], which is a very
good
> thing.
> >
> > The main difference I see is that various pieces of code will use a
boolean
> > directly, or a boolean test and the sense of the code isn't clear in
that
> case.
> > For example, in another piece of code in this CL, there's a boolean
field
> called
> > "fetch" which is passed directly as the resolve argument; and in
> > MergeAndWriteMe
> >
> > I'd leave ResolveFlag, but explicitly use the named constants
everywhere,
> > rather than just type converting to ResolveFlag.
> >
> > But given that this function isn't long for this world (I'm hoping),
I'm ok as
> > it is.
> >
> >
https:/
> > File environs/
> >
> >
https:/
> > environs/
gc.DeepEquals,
> > test.merged)
> > Thanks - I find this much easier to follow (and it's easier to
change if we
> > decide to change the semantics of MergeMetadata)
https:/