Code review comment for lp:~robru/bileto/publish-job

On Aug 22, 2016 10:15 PM, "Martin Pitt" <email address hidden> wrote:
>
> Robert Bruce Park [2016-08-22 16:06 -0000]:
> > Can you elaborate what you mean by "recursively call the method on its
direct children"?
>
> Well, I don't know what this is doing, but you said that __getattr__()
> has some magic to do that kind of iteration.

So a ticket can have many packages. When you "build a ticket" you want to
build each package. The manager class exists to ensure the classes
representing the packages are correctly instantiated and then passes
messages back and forth. So when you call manager.build() you're really
calling .build() on every package instance. Manager.__gettattr__() contains
the logic for calling the method, in parallel, on each instance.

In some cases there's some "global" logic that operates once on the whole
ticket rather than per package, and that's where you see those manager
methods that call __getattr__, they're processing the return values that
came back from each package, etc. But there are other cases where you might
call "manager.foo()" and foo doesn't exist at all, it goes straight to
package.foo() on each package. The idea was that the manager is a sort of
transparent wrapper around the package instances, you can call package
methods on it and it passes the calls down to each instance. Eg, it's an
explicit design goal that you would often call methods on the manager
instance that do not exist, which is why __getattr__ is defined. It's just
unfortunate that so many methods require a bit of intervention before /
after the __getattr__ logic that you see so many methods calling
__gettattr__ with their own method name.

>
> > The reason that __getattr__ is defined to pass methods down to
> > children is that this is called from dozens of places (and many of
> > those cases it isn't overloaded at all, it's just a direct pass
> > through), so I'm not going to reimplement that in every single place
> > that needs to pass through. I'll probably rename __getattr__() to
> > call_on_all_packages().
>
> Doing "__getattr__ = call_on_all_packages" and replacing half of the
> calls with the other makes things even *more*
> confusing/implicit/surprising, so better revert that bit or fix it
> consistently.

What do you mean replacing half the calls? Nothing is calling
"self.__getattr__()" any longer so it's perfectly consistent. __getattr__
is kept for the cases when a nonexistent method is called on the manager
class. That has to stay because they chain together, eg, you can call
"manager.foo().bar()" and that calls foo and bar on all package instances.
Getting rid of __getattr__ would result in code like
"manager.call_on_all_packages('foo').call_on_all_packages('bar')", eg,
redundant slop.

>
> > There is an rsync server running continuously on the bileto
> > instance, and snakefruit polls it every 5 minutes looking for a new
> > file to read. Bileto doesn't "start" rsync.
>
> So I suggest using a lock file (in both the rsync process and bileto)
> to ensure to not run rsync while bileto is updating that file tree.
> Another common pattern is to do something like this:
>
> cp -al cur_tree new_tree
> <change new_tree>
> mv cur_tree old_tree; mv new_tree cur_tree
>
> You can't atomically rename an existing (non-empty) directory, but at
> least this is fairly quick and thus will work in almost every case.
> However, lock files are cheaper, simpler, and completely robust.

No, lock files and moving directories are both dramatic overkill. We're
talking about a directory that usually has 0 files in it, maybe once or
twice a day it will have one file. The snakefruit script that is consuming
this deletes the files off rsync after downloading them. Simply writing the
file outside the directory and then moving it in is atomic, and sufficient
for the need.

And I checked, the snakefruit script does just glob, so writing in that dir
isn't safe.

>
> > As I said this was decided by infinity and I'm not sure what would be
correct.
>
> Please ask him again what this should mean and add a big fat comment
> about it. The concept of "upload flashplugin-nonfree into universe"
> simply does not make sense -- asking questions about it might be some
> weird LP quirk, but it should at least be documented. You yourself
> don't even know what it does any more :-)

"any more", I never did understand it, this whole checkupload thing was
always just magic runes given to me as a security requirement. IIRC Steve
wrote it and then Adam decided on the package / component names.

Anyway I'll ask Adam.