Copilot commented on code in PR #10523:
URL: https://github.com/apache/ozone/pull/10523#discussion_r3416431782
##########
hadoop-ozone/ozone-manager/src/main/resources/webapps/ozoneManager/ozoneManager.js:
##########
@@ -173,10 +173,10 @@
templateUrl: 'ratis-events.html',
controller: function ($http) {
var ctrl = this;
- $http.get("jmx?qry=Hadoop:service=OzoneManager,name=OMMetrics")
+
$http.get("jmx?qry=Hadoop:service=OzoneManager,name=OzoneManagerInfo,component=ServerRuntime")
.then(function (result) {
var metrics = result.data.beans[0];
- var rawEvents = metrics['tag.RatisEvents'] ?
metrics['tag.RatisEvents'].split('\n') : [];
+ var rawEvents = (metrics && metrics['RatisEvents']) ?
metrics['RatisEvents'].split('\n') : [];
Review Comment:
`result.data.beans[0]` is accessed without checking that `beans` exists / is
non-empty. If the JMX query returns no beans (eg, during startup or on error),
this will throw and break the Ratis events page. Mirror the defensive check
used elsewhere in this file (eg, around OMMetrics queries).
##########
hadoop-hdds/server-scm/src/main/resources/webapps/scm/scm.js:
##########
@@ -30,10 +30,10 @@
templateUrl: 'ratis-events.html',
controller: function ($http) {
var ctrl = this;
-
$http.get("jmx?qry=Hadoop:service=StorageContainerManager,name=SCMMetrics")
+
$http.get("jmx?qry=Hadoop:service=StorageContainerManager,name=StorageContainerManagerInfo,component=ServerRuntime")
.then(function (result) {
var metrics = result.data.beans[0];
- var rawEvents = metrics['tag.RatisEvents'] ?
metrics['tag.RatisEvents'].split('\n') : [];
+ var rawEvents = (metrics && metrics['RatisEvents']) ?
metrics['RatisEvents'].split('\n') : [];
Review Comment:
`result.data.beans[0]` is used without verifying that `beans` exists / is
non-empty. If the JMX query yields no beans, this will throw and the UI will
not render. Add the same guard used in other web UI components when parsing JMX
responses.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMMXBean.java:
##########
@@ -82,4 +82,6 @@ public interface SCMMXBean extends ServiceRuntimeInfo {
* @return the SCM hostname for the datanode.
*/
String getHostname();
+
+ String getRatisEvents();
Review Comment:
The newly added MXBean attribute `getRatisEvents()` lacks Javadoc. Please
document what it contains and the expected string format so external JMX/UI
consumers can parse it reliably.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMXBean.java:
##########
@@ -41,4 +41,6 @@ public interface OMMXBean extends ServiceRuntimeInfo {
* @return the OM hostname for the datanode.
*/
String getHostname();
+
+ String getRatisEvents();
Review Comment:
The newly added MXBean attribute `getRatisEvents()` should have Javadoc like
other MXBean attributes, and documenting the expected format (newline-separated
`timestamp|description`) will help UI/test consumers interpret it correctly.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3307,6 +3307,11 @@ public String getHostname() {
return omHostName;
}
+ @Override
+ public String getRatisEvents() {
+ return metrics != null ? metrics.getRatisEvents() : "";
+ }
Review Comment:
A new MXBean attribute (`RatisEvents`) is added and the OM web UI now
depends on it, but there is no test asserting that OzoneManagerInfo exposes
this JMX attribute (SCM has an integration test for this in the same PR).
Adding a small JMX-based test would prevent regressions in the
ObjectName/attribute wiring.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestSCMMXBean.java:
##########
@@ -86,6 +86,9 @@ public void testSCMMXBean() throws Exception {
double containerThreshold = (double) mbs.getAttribute(bean,
"SafeModeCurrentContainerThreshold");
assertEquals(scm.getCurrentContainerThreshold(), containerThreshold, 0);
+
+ String ratisEvents = (String) mbs.getAttribute(bean, "RatisEvents");
+ assertEquals(scm.getMetrics().getRatisEvents(), ratisEvents);
Review Comment:
This test now reaches into `scm.getMetrics().getRatisEvents()`, which
couples the MXBean contract to an internal implementation detail and can NPE if
metrics initialization changes. Since `StorageContainerManager` now exposes
`getRatisEvents()` directly (and handles `metrics == null`), assert against
that instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
