On 07/14/10 03:45 PM, [email protected] wrote:
...
On Tue, Jul 13, 2010 at 05:22:42PM -0700, Shawn Walker wrote:
On 07/13/10 11:53 AM, [email protected] wrote:
The webrev is here:
http://cr.opensolaris.org/~johansen/webrev-xpub-1/
...
======================================================================
src/modules/client/transport/repo.py:
======================================================================
line 598: missing spaces after '{' and before '}'. Also, it seems
like you could trivially add support for add to catalog here, by
(adding) and checking for X-IPkg-Add-To-Catalog.
I added the spaces, but I'm confused about the rest of this comment.
I re-implemented the transport parts here based upon what was in
FileTransaction and HTTPTransaction. The HTTPTransaction class didn't
support the add to catalog method. Are you saying that it should? Do I
need to make depot changes to enable this?
Right, the publication API didn't yet support add to catalog
(--no-catalog in pkgsend terms) but it's trivial to add this simply by
checking for the header in the transport system and in close_0 in
server/depot.py.
I know this is an RFE technically, but I felt since with very little
code we could add feature parity between file and http transport access
it was worth doing.
I'm not insisting you have to do this in this changeset, it could
certainly be deferred, but it seemed right to do this now while reving
the transport usage.
Note that for the same reasons we didn't rev /close/ (that is 0 -> 1)
for X-IPkg-Refresh-Index, I don't think we need to for
X-IPkg-Add-To-Catalog.
...
line 1197: s/Returns a transaction-ID/Returns the identifier
string for the transaction/
That's a mouthful. How about, "Returns the trasaction-ID string?"
That's fine.
======================================================================
src/modules/client/transport/transport.py:
======================================================================
...
lines 142-143: This can be:
return (p for p in self.__publishers.values())
Ok, but doesn't that make it less obvious that this is a generator,
since the parens are used in place of yield?
Not really; I use it lots of places in the catalog code and I think Bart
and Danek have used it a few places as well.
The key here is the parentheses; if you use an iterator expression in a
set of parentheses it will always be returned as a generator.
lines 1779-1783: simplified to the following?
repolist = repo.origins[:]
if not origin_only:
repolist.extend(repo.mirrors)
Changed.
This works for the __chunk_size case, but for gen_repos we actually
explicitly want to put mirrors first, just in case all entries are
equal. FWIW, this code isn't actually changed. Take a look at the
Sdiff to be certain.
That's fine.
line 1961: extra newline
Removed.
line 2051: so why cycle through origins for publication operations?
it seems like we should never have more than one origin for
publication. Same for other publication opeartions on 2096, etc.
The __gen_origins code is also responsible for retrying at repositories
where there are failures. This is used so that we retry a destination
more than once. Multiple destinations probably isn't a good idea.
Ah, ok. A comment stating that the iteration was to ensure we retry for
failure as opposed to retrying different origins would be helpful.
Plus, there should probably be an assertion somewhere that there is only
one origin for publication operations.
...
======================================================================
src/util/distro-import/importer.py:
======================================================================
...
line 1406: replace 'lib/pkg.depotd' with 'bin/pkgrepo'; also update
docstring on line 1404
line 1407: replace with:
argstr = "%s -s %s refresh" % (cmdname, path_to_repo)
These two are non-trivial changes to validate. It took me a full day
just to get the importer working to the point where I could run a test.
Since I've already tested this with pkg.depotd, I'd prefer not to change
this since I would have to re-test it. That takes hours to get set up.
I suppose since the importer is going away any day now that's ok, but we
should eventually change this. If you want to file a bug and move on
that's ok with me too.
...
======================================================================
src/util/publish/merge.py:
======================================================================
lines 182-231: The code in these seems common to our publication
utilities. Any way to centralise these somewhere? (misc? a new
module? perhaps pkg.util?) I realise the globals make this
difficult.
There have been some subtle variations in publisher setup between merge,
importer, and pull. I thought about commonizing this code, but I was
concerned that it would lead to a weirder method shared amongst them
all.
Ok, if you can find a way to do it without burning much time, I'm happy.
If not, move on...
Cheers,
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
