[Kde-pim] Review Request 112635: GID migration.

Tue Sep 10 19:17:14 BST 2013

> On Sept. 10, 2013, 5:19 p.m., Dan Vrátil wrote:
> > migration/gid/gidmigrationjob.cpp, line 62
> > <http://git.reviewboard.kde.org/r/112635/diff/1/?file=188580#file188580line62>
> >
> >     I'm afraid this will pile tons of ItemModifyJobs and execute them one by one only after the fetch job finishes.
> >     
> >     Maybe you could just store the items in a list and after the FetchJob finishes, you could process them one by one, like you do in GidMigrationJob::processCollection()? It should not have a big performance impact, but it will prevent the application consuming too much memory.
> >

I don't see why a modify job would take up significantly more space than the item with the full payload. The modifyjob could even be smart and drop the payload once it realizes it's ignored anyways and the GID is extracted. Are you sure this is an issue?

> On Sept. 10, 2013, 5:19 p.m., Dan Vrátil wrote:
> > migration/gid/gidmigrationjob.cpp, line 112
> > <http://git.reviewboard.kde.org/r/112635/diff/1/?file=188580#file188580line112>
> >
> >     When an error occurs, the GidMigrationJob will hang forever. There should be an else branch that re-emits the error from UpdateJob to this job and calls emitResult()

I think KCompositeJob takes already care of that, no?
See KCompositeJob::slotResult.

> On Sept. 10, 2013, 5:19 p.m., Dan Vrátil wrote:
> > migration/gid/gidmigrator.cpp, line 36
> > <http://git.reviewboard.kde.org/r/112635/diff/1/?file=188582#file188582line36>
> >
> >     I don't know whether showing users mimetype is a good idea.
> >     
> >     Also, %1 is the first argument :)

Well, the idea is that we will have a GidMigrator instantiated for each mimetype because that's how extractors are added. Because also the state is saved per migrator I figured this would be a useful granularity.

Soooo, the mimetype seems like a useful information for distinguishing between multiple gidmigrators ;-)

Note that I'm really not sure if any of that information should be exposed to the user.
I use that information in the control interface of the agent, but I rather see this as a tool for advanced users, respectively for cases where manual intervention is required. (So we could arguably drop i18n from those).

From a user perspective not much more information than "Akonadi is doing some background processing" should be leaking out (IMO).

> On Sept. 10, 2013, 5:19 p.m., Dan Vrátil wrote:
> > migration/migratorbase.h, line 32
> > <http://git.reviewboard.kde.org/r/112635/diff/1/?file=188587#file188587line32>
> >
> >     I'm not sure I understand point of this class...?

It implements the NullObject pattern. The point is that I don't have to  if (configIsAvailable()) { accessConfig() } throughout the code, but I can simply access the config and if non is available simply nothing happens.

This is mostly used for testing the stuff because it easily allows to inject a null config which persists nothing.

> On Sept. 10, 2013, 5:19 p.m., Dan Vrátil wrote:
> > migration/migratorbase.h, line 72
> > <http://git.reviewboard.kde.org/r/112635/diff/1/?file=188587#file188587line72>
> >
> >     Having KMigrationBase and MigrationBase is quite confusing...so maybe KDataMigrationBase, since this one is oriented on migrating data instead of resources? (just a suggestion, not an issue)

True enough.

MigrationBase should also be able to handle other migration tasks so we might be better of getting rid (or deprecating) KMigratorBase. I just didn't want to start with breaking existing stuff, especially since it's also being used from kmail code.

On a more general note, the whole MigrationBase/Agent combo could be used for arbitrary background processing where the jobs need some kind of scheduling and should be executed automatically. If there are other areas that could benefit from something like this we should think about generalizing it (The code wouldn't really need to change, it's just about the big picture and naming).

> On Sept. 10, 2013, 5:19 p.m., Dan Vrátil wrote:
> > migration/migratorbase.h, line 100
> > <http://git.reviewboard.kde.org/r/112635/diff/1/?file=188587#file188587line100>
> >
> >     Should we even show to users something like this? I mean all they need to care about is "oh, it's updating my stuff so that it's more awesome" - showing some technobabble will just confuse and scare them :)

Same as displayName(). I think it's useful for admins/powerusers, normal users should never even get to it.

> On Sept. 10, 2013, 5:19 p.m., Dan Vrátil wrote:
> > migration/migratorbase.cpp, line 137
> > <http://git.reviewboard.kde.org/r/112635/diff/1/?file=188588#file188588line137>
> >
> >     It should be documented that start() is blocking (or it should be made non-blocking, so that it's easier to run the Migration in a different thread)

I'll document it.

Isn't the whole point of the agent that we have a dedicated process to do background processing and that it's scheduler can decide the pace for processing it? It's IMO the schedulers job to decide if a migrator should run in a separate thread or not. So it would seem that migrators spawning threads would be counter productive.
So, not sure I understand the non-blocking idea but I'll document it ;-)

> On Sept. 10, 2013, 5:19 p.m., Dan Vrátil wrote:
> > migration/migratorbase.cpp, line 219
> > <http://git.reviewboard.kde.org/r/112635/diff/1/?file=188588#file188588line219>
> >
> >     Why not just store static_cast<int>(mMigrationState) ?

Maybe it's just me but I prefer to keep the semantics of each value tied to the enum and neither it's int value nor it's spelling (QMetaEnum or whatnot). The translation layer allows to keep working with the enum (reordering inserting new items, renaming the enums), without breaking existing config files or wreaking havoc when loading one. So I deliberately did not use it's int value.

- Christian

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112635/#review39733
-----------------------------------------------------------

On Sept. 10, 2013, 1:08 p.m., Christian Mollekopf wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112635/
> -----------------------------------------------------------
> 
> (Updated Sept. 10, 2013, 1:08 p.m.)
> 
> 
> Review request for KDEPIM.
> 
> 
> Description
> -------
> 
> GID migration.
> 
> A new MigratorBase as interface for migrators.
> 
> I didn't adapt the existing KMigratorBase as it taylored towards one migrator per resource and I found a couple of other incompatibilities so I figured I rather make something clean from scratch.
> 
> It should be easy enough to adapt KMigratorBase to be based on top MigratorBase if we still need that code.
> 
> 
> Diffs
> -----
> 
>   migration/CMakeLists.txt 3d15b97ab49a092e56416dce1c88725bcca3695e 
>   migration/gid/CMakeLists.txt PRE-CREATION 
>   migration/gid/gidmigrationjob.h PRE-CREATION 
>   migration/gid/gidmigrationjob.cpp PRE-CREATION 
>   migration/gid/gidmigrator.h PRE-CREATION 
>   migration/gid/gidmigrator.cpp PRE-CREATION 
>   migration/gid/main.cpp PRE-CREATION 
>   migration/infodialog.h 0003c48d3eab4260e4f0cc6a3ebf17ac762a1e1f 
>   migration/infodialog.cpp 6f08db23922c181b98d809d77b98564f7b6a3ca8 
>   migration/kmigratorbase.h 3a57917f0d0655320147ac13ef9544975c0188d0 
>   migration/migratorbase.h PRE-CREATION 
>   migration/migratorbase.cpp PRE-CREATION 
>   migration/tests/CMakeLists.txt a9129a6bd033867345a668c2fdda0c3c0adae7d6 
>   migration/tests/testgidmigration.h PRE-CREATION 
>   migration/tests/testgidmigration.cpp PRE-CREATION 
>   migration/tests/testmigratorbase.cpp PRE-CREATION 
>   migration/tests/unittestenv/config.xml fa0eafe88d573518ad72269f1b8aafb7f93c885a 
>   migration/tests/unittestenv/kdehome/share/config/akonadi_knut_resource_0rc PRE-CREATION 
>   migration/tests/unittestenv/kdehome/share/config/kdebugrc 32317f7453f4c0a0ec778bd494ced8e7d65078f2 
>   migration/tests/unittestenv/kdehome/testdata-res1.xml PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/112635/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Christian Mollekopf
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/