Expose server config, session queue and node as MBeans for JMX monitoring. #8838
Conversation
pujagani
force-pushed
the
expose-server-jmx
branch
from
891e043
to
c1713e6
Compare
| try { | ||
| return getExternalUri().toURL(); | ||
| } catch (MalformedURLException e) { | ||
| throw new ConfigException("Cannot convert URI to URL" + e.getMessage()); |
There was a problem hiding this comment.
nit: We probably want a space after URL
There was a problem hiding this comment.
👋 @pujagani, I went through it and I added a comment besides the one already exists.
Also, it seems tests for this PR are not passing, and a couple are still to be written. Could you please have a look? I'll happily review right after.
| @ManagedAttribute(name = "NewSessionQueueSize") | ||
| public int getQueueSize() { | ||
| Lock writeLock = lock.writeLock(); | ||
| writeLock.lock(); |
There was a problem hiding this comment.
Why do we need to lock to read the queue size?
There was a problem hiding this comment.
Since it uses ConcurrentLinkedDeque, the size() requires a traversal and value may not be accurate if it is being modified when size is calculated. Hence the lock. But for exposing values if we don't need that kind of accuracy, then we can get rid of the lock. @diemol What would you suggest?
|
@diemol I updated and added the tests and they all pass now. The test was picking up previously registered MBeans and hence failing for different cases. Please help review. Thank you! |
| @@ -80,6 +88,17 @@ public boolean isReady() { | |||
| return bus.isReady(); | |||
| } | |||
|
|
|||
| @ManagedAttribute(name = "NewSessionQueueSize") | |||
| public int getQueueSize() { | |||
| Lock writeLock = lock.writeLock(); | |||
| } | ||
|
|
||
| @ManagedAttribute(name = "RemoteNodeUri") | ||
| public URI getExternalURI() { |
There was a problem hiding this comment.
Nit: getExternalUri We try and AVOID SHOUTING in Selenium (which is why we have HttpMethod, and so on)
| @@ -101,6 +111,15 @@ public URI getExternalUri() { | |||
| } | |||
| } | |||
|
|
|||
| @ManagedAttribute(name = "URL") | |||
| public URL getExternalUrl() { | |||
There was a problem hiding this comment.
Daft question, but why don't we just return the URI?
|
|
||
| String retryInterval = (String) beanServer.getAttribute(name, "RetryIntervalSeconds"); | ||
| assertThat(Long.parseLong(retryInterval)).isEqualTo(queueOptions.getRetryIntervalSeconds()); | ||
| } catch (InstanceNotFoundException | IntrospectionException | ReflectionException |
There was a problem hiding this comment.
It's fine to just let the test fail because the exception was thrown. That'll give you more information that just calling fail in these catch blocks.
|
SonarCloud Quality Gate failed. |
Description
Changes are related to #7761.
Motivation and Context
Old Grid exposed MBeans for JMX monitoring. The changes include exposing MBeans and attributes for monitoring the new Grid. This includes:
Types of changes
Checklist