Modifications on ProcessInfo implementation

Thomas Segismont tsegismo at redhat.com
Fri Jan 18 14:08:45 UTC 2013


Hi,

Does anyone has any other concern about this? Otherwise I will merge it 
to master.

Thanks
Thomas

Le 08/01/2013 16:32, Thomas Segismont a écrit :
> Le 08/01/2013 16:08, Jay Shaughnessy a écrit :
>>
>> The analysis is good but  do we really need to change the API?  Or is it
>> really a situation of needing better documentation in the class? There
>> is a refresh(), is the problem that it is not called when appropriate?
>
> To be honest only adding documentation was also my first idea. But Lukas
> and I found that most of the time users of the class want up-to-date
> information (sometimes they use refresh but sometimes unfortunately they
> don't).
>
> So the idea of changing the API is making developers know just by
> reading the code that they are playing with cached data.
>
> It's also a way to get a one line instruction:
>
> processInfo.freshSnapshot().isRunning()
>
> instead of:
>
> if (processInfo.isRunning()) {
>    processInfo.refresh();
>    if (processInfo.isRunning()) {
> ...
>
> The above code actually exists (not exactly in this form).
>
>> Is freshSnapshot() (perhaps should be getFreshSnapshot()) the same as
>> calling refresh() and then the appropriate getter in the current API?
>>
>
> Yes it's the same. No problem to change the names if we find something
> more meaningful.
>
>> As an aside, know that CpuComponent already does a lot a special
>> processing to properly compute CPU usage.
>>
>
> I did not know about CpuComponent. Apparently it relies on
> CpuInformation which is indeed designed much like ProcessInfo is currently.
>
>> Whether we have new API or not, the class should be better documented
>> with the behavioral information presented in this e-mail thread.
>>
>
> I already added documentation in the bug branch which, for most parts,
> can be kept and if we stick with the original API.
>
>> On 1/8/2013 8:49 AM, Thomas Segismont wrote:
>>> Hi all,
>>>
>>> While working on a fix for BZ 885664 (OpenSSHD and MySQL availability
>>> check may report stale data) we started to have a discussion with
>>> Lukas on how the current ProcessInfo is implemented.
>>>
>>> ProcessInfo uses SIGAR to gather information on platform processes. It
>>> behaves like a cache of the SIGAR call results. So, when a user gets
>>> an existing ProcessInfo instance and invokes one of its methods,
>>> generally no new SIGAR call is made.
>>>
>>> This behavior (and the corresponding API) is not really documented and
>>> it has led to bugs like the BZ 885664 (the cached ProcState instance
>>> reports the underlying process is up even if it no longer exists).
>>>
>>> Lukas suggested I should try to make usage of the class less error
>>> prone while fixing the bug so I have updated the implementation in
>>> this way:
>>> * created a public internal class ProcessInfoSnapshot, which groups
>>> non static process property accessors (like state, CPU usage) and
>>> operations on these properties (like isRunning method)
>>> * added two new methods, priorSnapshot and freshSnapshot, to get the
>>> last or retrieve a new ProcessInfoSnapshot respectively.
>>> * kept static process properties and associated operations in the top
>>> level type
>>> * kept the previous API but marked it deprecated.
>>>
>>> If you want to see the changeset:
>>> http://git.fedorahosted.org/cgit/rhq/rhq.git/commit/?h=bug/885664&id=c40606d165efabf4091ebe506d4cf7ea2f5c098e
>>>
>>>
>>>
>>> As an example, to check if a process is alive with the new API:
>>>
>>> processInfo.freshSnapshot().isRunning();
>>>
>>> What do you think?
>>>
>>> Regards
>>> Thomas
>>> _______________________________________________
>>> rhq-devel mailing list
>>> rhq-devel at lists.fedorahosted.org
>>> https://lists.fedorahosted.org/mailman/listinfo/rhq-devel
>>
>> _______________________________________________
>> rhq-devel mailing list
>> rhq-devel at lists.fedorahosted.org
>> https://lists.fedorahosted.org/mailman/listinfo/rhq-devel
>
>
> _______________________________________________
> rhq-devel mailing list
> rhq-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/rhq-devel



More information about the rhq-devel mailing list