We discussed this with Thomas and actually it was me who advocated
the API
change. Mainly because of the fact that virtually all places that we
encountered misused the ProcessInfo.isRunning() method - all of the
places in
various plugins (that WE authored) assumed that isRunning returns
fresh data
without calling refresh() to actually fetch the fresh data.
Also note that as far as I looked, Thomas' changes don't break the
API - the previous way of using the class is still possible, but deprecated.
This is key. We need to make sure our api checking jobs are catching any changes here and
that we don't make any backward incompatible changes
On Tuesday, January 08, 2013 10:08:56 Jay Shaughnessy wrote:
> 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?
> Is freshSnapshot() (perhaps should be getFreshSnapshot()) the same
> as
> calling refresh() and then the appropriate getter in the current
> API?
>
> As an aside, know that CpuComponent already does a lot a special
> processing to properly compute CPU usage.
>
> Whether we have new API or not, the class should be better
> documented
> with the behavioral information presented in this e-mail thread.
>
> 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=...
> > 6d165efabf4091ebe506d4cf7ea2f5c098e
> >
> >
> > 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(a)lists.fedorahosted.org
> >
https://lists.fedorahosted.org/mailman/listinfo/rhq-devel
>
> _______________________________________________
> rhq-devel mailing list
> rhq-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/mailman/listinfo/rhq-devel
_______________________________________________
rhq-devel mailing list
rhq-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/rhq-devel