Brock Pytlik wrote: > John Rice wrote: >> Great that looks simpler again - we'll make the changes when are back >> in on Tuesday, test to make sure this does what's needed for both the >> GUI clients and respin. If Danek hasn't found a fix for the >> __init__.py issues we'll revert this file to r590. >> >> Bug logged to track root cause analysis of problem post 2008.11 (4116 >> depends on this): >> 4231:Resolve root cause for memory leak and remove workaround for 4116 >> >> Brock you happy with this approach, as opposed to modifying the API? >> > Actually, I find modifying the api cleaner than this. This just makes > my skin crawl personally. I'm not inherently opposed to leaving the > API workaround in for a while we get things fixed up. I think an API > option of "prefer low memory use over blazing speed" is at least a > possibly reasonable switch to provide. Himm see what you mean - at this point I just want it fixed for 2008.11 and the root cause looked into after that.
Can you and Shawn discuss so we can reach some consensus on what to do. Either solution will address our needs at the minute. I must admit I do like the idea of being able to control caching behavior in the clients via the api as the usage patterns and memory speed tradeoffs are quite different between clients that do one task and exit as opposed to allowing multiple tasks to be accomplished over an extended period of time on the same image. It would seem that turning on/off caching on the image is sensible, with the possibility of overriding it on a per operation basis if need be via an optional param. Something to consider. > Hard coding in our pkg client name here seems wrong to me on many > levels. If this is what everyone else likes, I'm fine with it for > 2008.11, but this workaround I want out of the code as soon as possible. > > Brock >> JR >> >> Shawn Walker wrote: >> >>> jmr wrote: >>> >>>> Michal has respun his webrev without the actions:__init__.py revert >>>> to r590 as well, to give us both options. >>>> >>>> With __init__.py from the trunk: >>>> http://cr.opensolaris.org/~migi/23_10_2008_bug_4116_v1_no_init/ >>>> >>>> With __init__.py from rev590: >>>> http://cr.opensolaris.org/~migi/23_10_2008_bug_4116_v1/ >>>> >>> Earlier today, it occurred to me that we know which pkg client is >>> currently using the API via history. As such, rather than add a >>> temporary flag to the api to control cache usage, unintentionally >>> exposing it, it makes more sense to just limit the caching to the >>> cli as a temporary workaround instead. >>> >>> To do this, I would remove all of the use_cache changes you've made >>> and make the following simple changes to modules/client/image.py: >>> >>> diff -r c95448e970e5 src/modules/client/image.py >>> --- a/src/modules/client/image.py Fri Oct 24 15:36:26 2008 -0500 >>> +++ b/src/modules/client/image.py Fri Oct 24 15:42:28 2008 -0500 >>> @@ -935,11 +935,14 @@ >>> """Find on-disk manifest and create in-memory Manifest >>> object, applying appropriate filters as needed.""" >>> >>> - if fmri in self.__manifest_cache: >>> - m = self.__manifest_cache[fmri] >>> + if self.history.client_name == "pkg": >>> + if fmri in self.__manifest_cache: >>> + m = self.__manifest_cache[fmri] >>> + else: >>> + m = self.__get_manifest(fmri) >>> + self.__manifest_cache[fmri] = m >>> else: >>> m = self.__get_manifest(fmri) >>> - self.__manifest_cache[fmri] = m >>> >>> self.__touch_manifest(fmri) >>> >>> >>> ========================== >>> >>> The above change is only intended to be temporary, and a bug would >>> need to be filed to address the real issue. However, this to me, is >>> far simpler and preferable to adding a use_cache flag which also >>> bumps the API version and so on. >>> >>> Code Review follows: >>> src/packagemanager.py: >>> line 40: s/lenght/length/ >>> >>> lines 580, 992-994: Suggested word change s/Fetching/Retrieving/ >>> >>> line 1112: tue users? what's that? >>> >>> Otherwise, looks fine. >>> >>> Cheers, >>> >> >> _______________________________________________ >> pkg-discuss mailing list >> [email protected] >> http://mail.opensolaris.org/mailman/listinfo/pkg-discuss >> > _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
