Re: [Gnumed-devel] Re: commits

[Karsten Hilbert]

> >2) Doing it this way will get you a call-by-reference result,
> >  IOW you will get a reference to the original encounter
> >  list, not a copy. Now, although you sorted the encounters
> >  after retrieving them this is not guaruanteed to be stable.
> >  Other callers getting a reference may have sorted differently.
> >  Hence we need to sort *when needed* despite the performance
> >  hit. However, the list of encounters isn't going to be
> >  particularly large. It'll be in the order of, what, ten to
> >  200 or so ?
> >
> You're saying get_encounters() uses self.__db_cache['encounters'] and 
> therefore returns a reference to the same list for both methods ?
Correct.

> But  the changes are  just a straight refactoring of the original code 
> that was there ( see below), so
> the  sychronization bug you're concerned with *was already there* ( I 
> didn't introduce the sorting).
What does refactoring do ? It moves parts of code to other
places within a body of code. Does that mean the same code is
executed ? Yes. Does it imply that the same code is executed
at the same time in the flow of code ? No.

The original code sorted *just before* depending on the sort
order. And it sorted *each time* it had to depend on that
order (I know that isn't desirable). The refactored code sorts
once after retrieving the source data. It depends on that sort
order "later" which can be in the range of a few milliseconds
to a few hours or even days in the extreme. That surely opens
a larger gap for sync issues. Putting a lock or sync directive
around that does not solve the issue either since we *do* want
other code to be able to work on the data structure in
question. One solution would be to cache the result and reuse
the cached values until the cache is evicted following backend
changes. Doable particularly given Ian's recent caching
additions but probably overkill (I might be wrong there as I
have no numbers).

> >4) What are those assignments supposed to achieve ?

> >        #--------------------------------------------------------
> >        def _get_encounters(self, issue_id=None, episode_id=None):
> >                h_iss = issue_id
> >                epis = episode_id
> >                if issue_id is not None:
> >                        h_iss = [issue_id]
> >                if episode_id is not None:
> >                        epis = [episode_id]
> >                encounters = self.get_encounters(issues=h_iss, 
> > episodes=epis)   # *1
> >                if encounters is None or len(encounters) == 0:
> >                        episodes = epis
> >                        issues = h_iss
> >                        _log.Log(gmLog.lErr, 'cannot retrieve first 
> > encounter for episodes [%s], issues [%s] (patient ID [%s])' % 
> > (str(episodes), str(issues), self.id_patient))
> >                        return [None]
> >                # FIXME: this does not scale particularly well
> >                encounters.sort(lambda x,y: cmp(x['started'], y['started']))
> >                return encounters
> 
> Wrt to episodes and issues,  I'm not sure if parameter assignment in 
> python  means a local variable  is created as well
> ie. Does  "encounters = self.get_encounters(issues=h_iss, 
> episodes=epis)"  create the issues and episodes variables used in the error 
> log messages parameters ?
No it does not and that was a bug in the original code. The
fix is to use the existing local variables h_iss and epis
instead of issues/episodes. Fixed.

> I remember getting some sort of error  thrown from the log messaging call.
Which was a symptom of the bug.

> The bug is in the original code
Correct.

Karsten
-- 
GPG key ID E4071346 @ wwwkeys.pgp.net
E167 67FD A291 2BEA 73BD  4537 78B9 A9F9 E407 1346