Re: Web Console: Portlet Review
Cool stuff.
For the initial kernel reference you can get it from the JNDI ENC.
Just add an env entry of type org.apache.geronimo.kernel.Kernel
with no value and the deployment code will automatically put the
kernel there.
-dain
On Jul 22, 2005, at 10:11 PM, Aaron Mulder wrote:
Well, I actually worked with a couple portlets today. For
starters, I think the diff at the bottom of this message speaks
volumes
about why a management API based on interfaces is nice. It takes out
Strings, constants to hold the Strings, the Kernel variable, and
various
casts, and instead stores things like the current Server and JVM in
the
session and queries them to get the data it needs. The code is
smaller
and more reliable. :)
As for the old code, I'm not thrilled with it even besides the
kernel invocations, for a couple reasons (and I just looked at the two
portlets on the front Server Info page so far):
- The portlets that need to use a Kernel keep one in a local
variable,
and populate it by manually looking up a kernel during
initialization. I
would really have preferred to see common get kernel logic at
least, if
not a common utility to hold the current kernel.
- The portlets assume they are running within the server in question,
doing things like calling System.getProperties to get the server's
system properties (and also using a local kernel, of course). While I
have to agree that it works for now, I don't think it's any harder to
write this so it would work against a remote server, and it ends up
being
cleaner anyway.
- The portlets store data in static variables, even though they
are not
reused from request to request. My J2EE application multithreading
sensors are going off, though I don't know for sure whether a portlet
instance can service more than one request at a time. But I also
don't
like using static variables for what is otherwise a totally stateless
class, and there's especially no reason to if they are reassigned
at the
beginning of every request.
- Many of the kernel invocations returning objects are just popped
into a
collection without casting, which is almost as bad as casting, because
then there's no assurance that if the JSP needed a particular type
to be
there, the value would be of the right type. Of course the JSPs I
looked
just print objects and don't care about type.
- The JSP for printing system properties, for example, has
separate lines
for every individual property, and manually sets the background
style for
each, to achieve the alternating row color. There are just so many
better
ways to do this, that take advantage of, say, a loop, and don't
involve
needing to manually restyle every row after inserting a new row,
and so
on.
Anyway, I think the portlets are going to need a bit of attention.
That's OK I guess, since I'd like to migrate most of them to the new
management API anyway, so we'll have occasion to touch them, but I
figured
I'd give my thoughts on where things stand.
Thanks,
Aaron
Index: console-standard/src/java/org/apache/geronimo/console/
infomanager/ServerInfoPortlet.java
===
--- console-standard/src/java/org/apache/geronimo/console/
infomanager/ServerInfoPortlet.java(revision 224404)
+++ console-standard/src/java/org/apache/geronimo/console/
infomanager/ServerInfoPortlet.java(working copy)
@@ -22,7 +22,6 @@
import java.util.HashMap;
import java.util.Map;
-import javax.management.ObjectName;
import javax.portlet.ActionRequest;
import javax.portlet.ActionResponse;
import javax.portlet.GenericPortlet;
@@ -33,56 +32,22 @@
import javax.portlet.RenderResponse;
import javax.portlet.WindowState;
-import org.apache.geronimo.console.util.ObjectNameConstants;
-import org.apache.geronimo.kernel.Kernel;
-import org.apache.geronimo.kernel.KernelRegistry;
+import org.apache.geronimo.console.util.PortletManager;
+import org.apache.geronimo.j2ee.management.geronimo.JVM;
+/**
+ * Calculates various information about the server to display in
the server
+ * info portlet view (on of several JSPs depending on the portlet
state).
+ *
+ * @version $Rev: 46019 $ $Date: 2004-09-14 05:56:06 -0400 (Tue,
14 Sep 2004) $
+ */
public class ServerInfoPortlet extends GenericPortlet {
-
-public static final String SVRINFO_BASEDIR = baseDirectory;
-
-public static final String SVRINFO_VERSION = version;
-
-public static final String SVRINFO_BUILDDATE = buildDate;
-
-public static final String SVRINFO_BUILDTIME = buildTime;
-
-public static final String SVRINFO_COPYRIGHT = copyright;
-
-public static final String SVRINFO_PLATFORMARCH = platformArch;
-
-public static final String SVRINFO_GERONIMO_BUILD_VERSION =
geronimoBuildVersion;
-
-public static final String SVRINFO_GERONIMO_SPEC_VERSION =
geronimoSpecVersion;
-
-public static final String SVRINFO_PORTAL_CORE_VERSION =
Web Console: Portlet Review
Well, I actually worked with a couple portlets today. For
starters, I think the diff at the bottom of this message speaks volumes
about why a management API based on interfaces is nice. It takes out
Strings, constants to hold the Strings, the Kernel variable, and various
casts, and instead stores things like the current Server and JVM in the
session and queries them to get the data it needs. The code is smaller
and more reliable. :)
As for the old code, I'm not thrilled with it even besides the
kernel invocations, for a couple reasons (and I just looked at the two
portlets on the front Server Info page so far):
- The portlets that need to use a Kernel keep one in a local variable,
and populate it by manually looking up a kernel during initialization. I
would really have preferred to see common get kernel logic at least, if
not a common utility to hold the current kernel.
- The portlets assume they are running within the server in question,
doing things like calling System.getProperties to get the server's
system properties (and also using a local kernel, of course). While I
have to agree that it works for now, I don't think it's any harder to
write this so it would work against a remote server, and it ends up being
cleaner anyway.
- The portlets store data in static variables, even though they are not
reused from request to request. My J2EE application multithreading
sensors are going off, though I don't know for sure whether a portlet
instance can service more than one request at a time. But I also don't
like using static variables for what is otherwise a totally stateless
class, and there's especially no reason to if they are reassigned at the
beginning of every request.
- Many of the kernel invocations returning objects are just popped into a
collection without casting, which is almost as bad as casting, because
then there's no assurance that if the JSP needed a particular type to be
there, the value would be of the right type. Of course the JSPs I looked
just print objects and don't care about type.
- The JSP for printing system properties, for example, has separate lines
for every individual property, and manually sets the background style for
each, to achieve the alternating row color. There are just so many better
ways to do this, that take advantage of, say, a loop, and don't involve
needing to manually restyle every row after inserting a new row, and so
on.
Anyway, I think the portlets are going to need a bit of attention.
That's OK I guess, since I'd like to migrate most of them to the new
management API anyway, so we'll have occasion to touch them, but I figured
I'd give my thoughts on where things stand.
Thanks,
Aaron
Index:
console-standard/src/java/org/apache/geronimo/console/infomanager/ServerInfoPortlet.java
===
---
console-standard/src/java/org/apache/geronimo/console/infomanager/ServerInfoPortlet.java
(revision 224404)
+++
console-standard/src/java/org/apache/geronimo/console/infomanager/ServerInfoPortlet.java
(working copy)
@@ -22,7 +22,6 @@
import java.util.HashMap;
import java.util.Map;
-import javax.management.ObjectName;
import javax.portlet.ActionRequest;
import javax.portlet.ActionResponse;
import javax.portlet.GenericPortlet;
@@ -33,56 +32,22 @@
import javax.portlet.RenderResponse;
import javax.portlet.WindowState;
-import org.apache.geronimo.console.util.ObjectNameConstants;
-import org.apache.geronimo.kernel.Kernel;
-import org.apache.geronimo.kernel.KernelRegistry;
+import org.apache.geronimo.console.util.PortletManager;
+import org.apache.geronimo.j2ee.management.geronimo.JVM;
+/**
+ * Calculates various information about the server to display in the server
+ * info portlet view (on of several JSPs depending on the portlet state).
+ *
+ * @version $Rev: 46019 $ $Date: 2004-09-14 05:56:06 -0400 (Tue, 14 Sep 2004) $
+ */
public class ServerInfoPortlet extends GenericPortlet {
-
-public static final String SVRINFO_BASEDIR = baseDirectory;
-
-public static final String SVRINFO_VERSION = version;
-
-public static final String SVRINFO_BUILDDATE = buildDate;
-
-public static final String SVRINFO_BUILDTIME = buildTime;
-
-public static final String SVRINFO_COPYRIGHT = copyright;
-
-public static final String SVRINFO_PLATFORMARCH = platformArch;
-
-public static final String SVRINFO_GERONIMO_BUILD_VERSION =
geronimoBuildVersion;
-
-public static final String SVRINFO_GERONIMO_SPEC_VERSION =
geronimoSpecVersion;
-
-public static final String SVRINFO_PORTAL_CORE_VERSION =
portalCoreVersion;
-
-public static final String JVMIMPL_JAVAVER = javaVersion;
-
-public static final String JVMIMPL_JAVAVENDOR = javaVendor;
-
-public static final String JVMIMPL_NODE = node;
-
-public static final String JVMIMPL_FREEMEM = freeMemory;
-
-public static final String
