Modifications on ProcessInfo implementation

Thomas Segismont tsegismo at redhat.com
Thu Jan 24 17:33:53 UTC 2013


Hi,

These changes are now in master as part of the BZ 885664 [1] fix.

The changeset also includes a new refresh guard as I figured out that 
Sigar#getProcState may return wrong value on consecutive calls. I posted 
a message [2] to SIGAR user forum for that issue.

The changeset has been tested with probe and api-check Jenkins jobs (no 
API changes found).

Thanks and regards
Thomas

[1] https://bugzilla.redhat.com/show_bug.cgi?id=885664
[2] http://communities.vmware.com/message/2183651#2183651

Le 18/01/2013 15:08, Thomas Segismont a écrit :
> 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