Modifications on ProcessInfo implementation

Thomas Segismont tsegismo at redhat.com
Tue Jan 8 15:32:40 UTC 2013


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




More information about the rhq-devel mailing list