Mercurial > hg > release > thermostat-0.13
changeset 1213:d44f06cf8a58
regression: no jmx notifications displayed on client
Reviewed-by: jerboaa
Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2013-August/007840.html
line wrap: on
line diff
--- a/common/command/src/main/java/com/redhat/thermostat/common/command/Request.java Thu Jul 25 17:45:33 2013 +0200 +++ b/common/command/src/main/java/com/redhat/thermostat/common/command/Request.java Wed Aug 14 10:38:38 2013 -0400 @@ -154,7 +154,7 @@ @Override public String toString() { - return "{ Request: {target = " + target.toString() + "}, {type = " + type.name() + "} }"; + return "{ Request: {target = " + target.toString() + "}, {type = " + type.name() + "}, {parameters = " + parameters.toString() + "} }"; } }
--- a/common/command/src/test/java/com/redhat/thermostat/common/command/RequestTest.java Thu Jul 25 17:45:33 2013 +0200 +++ b/common/command/src/test/java/com/redhat/thermostat/common/command/RequestTest.java Wed Aug 14 10:38:38 2013 -0400 @@ -115,7 +115,7 @@ public void testToString() { InetSocketAddress addr = new InetSocketAddress(1234); Request request = new Request(RequestType.RESPONSE_EXPECTED, addr); - assertEquals("{ Request: {target = "+ addr.toString() + "}, {type = RESPONSE_EXPECTED} }", request.toString()); + assertEquals("{ Request: {target = "+ addr.toString() + "}, {type = RESPONSE_EXPECTED}, {parameters = {}} }", request.toString()); } }
--- a/vm-jmx/agent/src/main/java/com/redhat/thermostat/vm/jmx/agent/internal/JmxBackend.java Thu Jul 25 17:45:33 2013 +0200 +++ b/vm-jmx/agent/src/main/java/com/redhat/thermostat/vm/jmx/agent/internal/JmxBackend.java Wed Aug 14 10:38:38 2013 -0400 @@ -83,6 +83,17 @@ private boolean isActive = false; + // Used as a callback handler + private static class VmIdAndPid { + public final String vmId; + public final int pid; + + public VmIdAndPid(String vmId, int pid) { + this.vmId = vmId; + this.pid = pid; + } + } + public JmxBackend(Version version, ReceiverRegistry registry, JmxNotificationDAO dao, MXBeanConnectionPool pool, RequestReceiver receiver) { this(version, registry, dao, pool, receiver, new SystemClock()); } @@ -135,7 +146,8 @@ return isActive; } - public void enableNotificationsFor(int pid) { + public void enableNotificationsFor(String vmId, int pid) { + VmIdAndPid idAndPid = new VmIdAndPid(vmId, pid); try { MXBeanConnection connection = pool.acquire(pid); connections.put(pid, connection); @@ -143,13 +155,13 @@ Set<ObjectName> names = server.queryNames(null, null); for (ObjectName name : names) { if (name.equals(MBeanServerDelegate.DELEGATE_NAME)) { - server.addNotificationListener(MBeanServerDelegate.DELEGATE_NAME, this.registrationNotificationListener, null, pid); + server.addNotificationListener(MBeanServerDelegate.DELEGATE_NAME, this.registrationNotificationListener, null, idAndPid); } else { - addNotificationListenerToMBean(pid, server, name); + addNotificationListenerToMBean(idAndPid, server, name); } } JmxNotificationStatus update = new JmxNotificationStatus(); - update.setVmId(pid); + update.setVmId(vmId); update.setEnabled(true); update.setTimeStamp(clock.getRealTimeMillis()); dao.addNotificationStatus(update); @@ -158,11 +170,11 @@ } } - public void disableNotificationsFor(int pid) { + public void disableNotificationsFor(String vmId, int pid) { MXBeanConnection connection = connections.get(pid); JmxNotificationStatus update = new JmxNotificationStatus(); - update.setVmId(pid); + update.setVmId(vmId); update.setEnabled(false); update.setTimeStamp(clock.getRealTimeMillis()); dao.addNotificationStatus(update); @@ -178,15 +190,15 @@ @Override public void handleNotification(Notification notification, Object handback) { - Integer pid = (Integer) handback; - MBeanServerConnection server = connections.get(pid).get(); + VmIdAndPid idAndPid = (VmIdAndPid) handback; + MBeanServerConnection server = connections.get(idAndPid.pid).get(); MBeanServerNotification serverNotification = (MBeanServerNotification) notification; ObjectName name = serverNotification.getMBeanName(); try { if (MBeanServerNotification.REGISTRATION_NOTIFICATION.equals(serverNotification.getType())) { logger.fine("MBean Registered: " + name); - addNotificationListenerToMBean(pid, server, name); + addNotificationListenerToMBean(idAndPid, server, name); } else if (MBeanServerNotification.UNREGISTRATION_NOTIFICATION.equals(serverNotification.getType())) { logger.fine("MBean Unregistered: " + name); // we should remove the listener, but the object is not @@ -204,10 +216,10 @@ @Override public void handleNotification(Notification notification, Object handback) { - int pid = (Integer) handback; + VmIdAndPid idAndPid = (VmIdAndPid) handback; JmxNotification data = new JmxNotification(); - data.setVmId(pid); + data.setVmId(idAndPid.vmId); data.setImportance("unknown"); data.setTimeStamp(notification.getTimeStamp()); data.setSourceBackend(JmxBackend.class.getName()); @@ -218,10 +230,10 @@ } } - private void addNotificationListenerToMBean(int pid, MBeanServerConnection server, ObjectName name) + private void addNotificationListenerToMBean(VmIdAndPid idAndPid, MBeanServerConnection server, ObjectName name) throws InstanceNotFoundException, IntrospectionException, ReflectionException, IOException { if (server.getMBeanInfo(name).getNotifications().length > 0) { - server.addNotificationListener(name, JmxBackend.this.notificationWriter, null, pid); + server.addNotificationListener(name, JmxBackend.this.notificationWriter, null, idAndPid); } } }
--- a/vm-jmx/agent/src/main/java/com/redhat/thermostat/vm/jmx/agent/internal/JmxRequestListener.java Thu Jul 25 17:45:33 2013 +0200 +++ b/vm-jmx/agent/src/main/java/com/redhat/thermostat/vm/jmx/agent/internal/JmxRequestListener.java Wed Aug 14 10:38:38 2013 -0400 @@ -58,6 +58,7 @@ @Override public Response receive(Request request) { Response response = new Response(ResponseType.OK); + String vmId = request.getParameter(JmxCommand.VM_ID); String strPid = request.getParameter(JmxCommand.VM_PID); try { int pid = Integer.parseInt(strPid); @@ -65,10 +66,10 @@ switch (JmxCommand.valueOf(command)) { case DISABLE_JMX_NOTIFICATIONS: - backend.disableNotificationsFor(pid); + backend.disableNotificationsFor(vmId, pid); break; case ENABLE_JMX_NOTIFICATIONS: - backend.enableNotificationsFor(pid); + backend.enableNotificationsFor(vmId, pid); break; } } catch (NumberFormatException e) {
--- a/vm-jmx/agent/src/test/java/com/redhat/thermostat/vm/jmx/agent/internal/JmxBackendTest.java Thu Jul 25 17:45:33 2013 +0200 +++ b/vm-jmx/agent/src/test/java/com/redhat/thermostat/vm/jmx/agent/internal/JmxBackendTest.java Wed Aug 14 10:38:38 2013 -0400 @@ -147,7 +147,7 @@ when(connection.get()).thenReturn(actual); when(pool.acquire(42)).thenReturn(connection); - backend.enableNotificationsFor(42); + backend.enableNotificationsFor("42", 42); verify(actual).addNotificationListener(eq(name1), any(NotificationListener.class), eq((NotificationFilter) null), any()); } @@ -169,7 +169,7 @@ when(connection.get()).thenReturn(actual); when(pool.acquire(42)).thenReturn(connection); - backend.enableNotificationsFor(42); + backend.enableNotificationsFor("42", 42); ArgumentCaptor<NotificationListener> listenerCaptor = ArgumentCaptor.forClass(NotificationListener.class); ArgumentCaptor<Object> handbackCaptor = ArgumentCaptor.forClass(Object.class); @@ -212,7 +212,7 @@ when(connection.get()).thenReturn(actual); when(pool.acquire(42)).thenReturn(connection); - backend.enableNotificationsFor(42); + backend.enableNotificationsFor("42", 42); ArgumentCaptor<NotificationListener> listenerCaptor = ArgumentCaptor.forClass(NotificationListener.class); ArgumentCaptor<Object> handbackCaptor = ArgumentCaptor.forClass(Object.class); @@ -246,7 +246,7 @@ when(connection.get()).thenReturn(actual); when(pool.acquire(42)).thenReturn(connection); - backend.enableNotificationsFor(42); + backend.enableNotificationsFor("42", 42); ArgumentCaptor<NotificationListener> listenerCaptor = ArgumentCaptor.forClass(NotificationListener.class); ArgumentCaptor<Object> handbackCaptor = ArgumentCaptor.forClass(Object.class); @@ -256,7 +256,7 @@ NotificationListener listener = listenerCaptor.getValue(); verify(actual).queryNames(null, null); - verify(actual).addNotificationListener(eq(MBeanServerDelegate.DELEGATE_NAME), isA(NotificationListener.class), eq((NotificationFilter) null), isA(Integer.class)); + verify(actual).addNotificationListener(eq(MBeanServerDelegate.DELEGATE_NAME), isA(NotificationListener.class), eq((NotificationFilter) null), isA(Object.class)); listener.handleNotification(mBeanRemovedNotification, handbackCaptor.getValue()); @@ -274,9 +274,9 @@ when(connection.get()).thenReturn(actual); when(pool.acquire(42)).thenReturn(connection); - backend.enableNotificationsFor(42); + backend.enableNotificationsFor("42", 42); - backend.disableNotificationsFor(42); + backend.disableNotificationsFor("42", 42); verify(pool).release(42, connection); }
--- a/vm-jmx/agent/src/test/java/com/redhat/thermostat/vm/jmx/agent/internal/JmxRequestListenerTest.java Thu Jul 25 17:45:33 2013 +0200 +++ b/vm-jmx/agent/src/test/java/com/redhat/thermostat/vm/jmx/agent/internal/JmxRequestListenerTest.java Wed Aug 14 10:38:38 2013 -0400 @@ -72,10 +72,11 @@ Request req = new Request(RequestType.RESPONSE_EXPECTED, new InetSocketAddress(HOST, PORT)); req.setParameter(JmxCommand.class.getName(), JmxCommand.ENABLE_JMX_NOTIFICATIONS.name()); req.setParameter(JmxCommand.VM_PID, "42"); + req.setParameter(JmxCommand.VM_ID, "42"); Response result = requestListener.receive(req); - verify(backend).enableNotificationsFor(42); + verify(backend).enableNotificationsFor("42", 42); assertEquals(ResponseType.OK, result.getType()); } @@ -85,10 +86,11 @@ Request req = new Request(RequestType.RESPONSE_EXPECTED, new InetSocketAddress(HOST, PORT)); req.setParameter(JmxCommand.class.getName(), JmxCommand.DISABLE_JMX_NOTIFICATIONS.name()); req.setParameter(JmxCommand.VM_PID, "42"); + req.setParameter(JmxCommand.VM_ID, "42"); Response result = requestListener.receive(req); - verify(backend).disableNotificationsFor(42); + verify(backend).disableNotificationsFor("42", 42); assertEquals(ResponseType.OK, result.getType()); }
--- a/vm-jmx/client-core/src/main/java/com/redhat/thermostat/vm/jmx/client/core/internal/JmxNotificationsViewController.java Thu Jul 25 17:45:33 2013 +0200 +++ b/vm-jmx/client-core/src/main/java/com/redhat/thermostat/vm/jmx/client/core/internal/JmxNotificationsViewController.java Wed Aug 14 10:38:38 2013 -0400 @@ -165,6 +165,10 @@ @Override public void run() { JmxNotificationStatus status = dao.getLatestNotificationStatus(vm); + if (status == null) { + return; + } + notificationsEnabled.set(status.isEnabled()); view.setNotificationsEnabled(notificationsEnabled.get());
--- a/vm-jmx/client-core/src/main/java/com/redhat/thermostat/vm/jmx/client/core/internal/JmxToggleNotificationRequest.java Thu Jul 25 17:45:33 2013 +0200 +++ b/vm-jmx/client-core/src/main/java/com/redhat/thermostat/vm/jmx/client/core/internal/JmxToggleNotificationRequest.java Wed Aug 14 10:38:38 2013 -0400 @@ -86,6 +86,7 @@ req.setParameter(Request.ACTION, CMD_CHANNEL_ACTION_NAME); req.setParameter(JmxCommand.class.getName(), enable ? JmxCommand.ENABLE_JMX_NOTIFICATIONS.name() : JmxCommand.DISABLE_JMX_NOTIFICATIONS.name()); req.setParameter(JmxCommand.VM_PID, String.valueOf(vm.getPid())); + req.setParameter(JmxCommand.VM_ID, vm.getVmId()); JmxToggleResponseListener listener = factory.createListener(successAction, failureAction); req.addListener(listener);
--- a/vm-jmx/client-core/src/test/java/com/redhat/thermostat/vm/jmx/client/core/internal/JmxNotificationsViewControllerTest.java Thu Jul 25 17:45:33 2013 +0200 +++ b/vm-jmx/client-core/src/test/java/com/redhat/thermostat/vm/jmx/client/core/internal/JmxNotificationsViewControllerTest.java Wed Aug 14 10:38:38 2013 -0400 @@ -38,7 +38,9 @@ import static org.junit.Assert.assertEquals; import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyBoolean; import static org.mockito.Matchers.eq; +import static org.mockito.Matchers.isA; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -46,6 +48,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import java.util.ArrayList; import java.util.Arrays; import java.util.concurrent.ExecutorService; import java.util.concurrent.TimeUnit; @@ -176,6 +179,22 @@ } @Test + public void verifyTimerWorksWhenNoStatus() { + ArgumentCaptor<Runnable> actionCaptor = ArgumentCaptor.forClass(Runnable.class); + verify(timer).setAction(actionCaptor.capture()); + + Runnable timerAction = actionCaptor.getValue(); + + JmxNotification data = mock(JmxNotification.class); + when(notificationDao.getNotifications(vm, Long.MIN_VALUE)).thenReturn(new ArrayList<JmxNotification>()); + JmxNotificationStatus status = mock(JmxNotificationStatus.class); + when(notificationDao.getLatestNotificationStatus(vm)).thenReturn(null); + + verify(view, never()).setNotificationsEnabled(anyBoolean()); + verify(view, never()).addNotification(isA(JmxNotification.class)); + } + + @Test public void verifyTimerIsStartedWhenViewIsVisible() { ArgumentCaptor<ActionListener> listenerCaptor = ArgumentCaptor.forClass(ActionListener.class);
--- a/vm-jmx/client-core/src/test/java/com/redhat/thermostat/vm/jmx/client/core/internal/JmxToggleNotificationRequestTest.java Thu Jul 25 17:45:33 2013 +0200 +++ b/vm-jmx/client-core/src/test/java/com/redhat/thermostat/vm/jmx/client/core/internal/JmxToggleNotificationRequestTest.java Wed Aug 14 10:38:38 2013 -0400 @@ -120,6 +120,7 @@ assertEquals(JmxToggleNotificationRequest.CMD_CHANNEL_ACTION_NAME, req.getParameter(Request.ACTION)); assertEquals(JmxCommand.RECEIVER, req.getReceiver()); assertEquals(String.valueOf(vm.getPid()), req.getParameter(JmxCommand.VM_PID)); + assertEquals(vm.getVmId(), req.getParameter(JmxCommand.VM_ID)); assertEquals(JmxCommand.ENABLE_JMX_NOTIFICATIONS.name(), req.getParameter(JmxCommand.class.getName())); @@ -149,6 +150,7 @@ assertEquals(JmxToggleNotificationRequest.CMD_CHANNEL_ACTION_NAME, req.getParameter(Request.ACTION)); assertEquals(JmxCommand.RECEIVER, req.getReceiver()); assertEquals(String.valueOf(vm.getPid()), req.getParameter(JmxCommand.VM_PID)); + assertEquals(vm.getVmId(), req.getParameter(JmxCommand.VM_ID)); assertEquals(JmxCommand.DISABLE_JMX_NOTIFICATIONS.name(), req.getParameter(JmxCommand.class.getName()));
--- a/vm-jmx/common/src/main/java/com/redhat/thermostat/vm/jmx/common/JmxCommand.java Thu Jul 25 17:45:33 2013 +0200 +++ b/vm-jmx/common/src/main/java/com/redhat/thermostat/vm/jmx/common/JmxCommand.java Wed Aug 14 10:38:38 2013 -0400 @@ -42,6 +42,7 @@ DISABLE_JMX_NOTIFICATIONS, ; + public static final String VM_ID = "VM_ID"; public static final String VM_PID = "VM_PID"; public static final String RECEIVER = "com.redhat.thermostat.vm.jmx.agent.internal.JmxRequestListener"; }
--- a/vm-jmx/common/src/main/java/com/redhat/thermostat/vm/jmx/common/JmxNotification.java Thu Jul 25 17:45:33 2013 +0200 +++ b/vm-jmx/common/src/main/java/com/redhat/thermostat/vm/jmx/common/JmxNotification.java Wed Aug 14 10:38:38 2013 -0400 @@ -45,19 +45,19 @@ public class JmxNotification extends BasePojo implements TimeStampedPojo { private long timeStamp; - private int vmId; + private String vmId; private String importance; private String sourceBackend; private String sourceDetails; private String contents; @Persist - public int getVmId() { + public String getVmId() { return vmId; } @Persist - public void setVmId(int vmId) { + public void setVmId(String vmId) { this.vmId = vmId; }
--- a/vm-jmx/common/src/main/java/com/redhat/thermostat/vm/jmx/common/JmxNotificationDAO.java Thu Jul 25 17:45:33 2013 +0200 +++ b/vm-jmx/common/src/main/java/com/redhat/thermostat/vm/jmx/common/JmxNotificationDAO.java Wed Aug 14 10:38:38 2013 -0400 @@ -44,9 +44,19 @@ void addNotificationStatus(JmxNotificationStatus notificationsStatus); + /** + * Returns the latest (based on the timestamp) status for the given vm + * + * @return a {@link JmxNotificationStatus} representing the status or + * {@code null} if no such information could be located + */ JmxNotificationStatus getLatestNotificationStatus(VmRef statusFor); void addNotification(JmxNotification notification); + /** + * @return a {@link List} (may be empty) of all {@link JmxNotification}s + * matching the vm and with a timestamp greater than {@code timeStampSince} + */ List<JmxNotification> getNotifications(VmRef notificationsFor, long timeStampSince); }
--- a/vm-jmx/common/src/main/java/com/redhat/thermostat/vm/jmx/common/JmxNotificationStatus.java Thu Jul 25 17:45:33 2013 +0200 +++ b/vm-jmx/common/src/main/java/com/redhat/thermostat/vm/jmx/common/JmxNotificationStatus.java Wed Aug 14 10:38:38 2013 -0400 @@ -45,16 +45,16 @@ public class JmxNotificationStatus extends BasePojo implements TimeStampedPojo { private long timeStamp = 0; - private int vmId = 0; + private String vmId = null; private boolean enabled = false; @Persist - public int getVmId() { + public String getVmId() { return vmId; } @Persist - public void setVmId(int vmId) { + public void setVmId(String vmId) { this.vmId = vmId; }