Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HDDS-11563. [OM/SCM/DN] WebUI Display Namespace #7321

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

aierate
Copy link
Contributor

@aierate aierate commented Oct 16, 2024

What changes were proposed in this pull request?

We have enhanced the web UI to display cluster information for OM (Ozone Manager), SCM (Storage Container Manager), and DN (DataNode).

Please describe your PR in detail:

  • We have added the getNamespace interface to retrieve and display Namespace information on the web UI and JMX for OM, SCM, and DN. This enhances the ability to understand cluster information through the web UI.
  • It is located in the namespace field under the Overview title.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-11563

How was this patch tested?

It is Only WebUI.
SCM
图片
OM
图片
Datanode
图片

@slfan1989
Copy link
Contributor

LGTM.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @aierate for the patch.

@@ -26,4 +26,6 @@
*/
@InterfaceAudience.Private
public interface DNMXBean extends ServiceRuntimeInfo {

String getNamespace();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you would like to display "namespace" in the generic overview section, I think getNamespace() should be defined in ServiceRuntimeInfo, where the other generic properties (started/version/compiled) are defined.

Comment on lines 33 to 35
public String getNamespace() {
return "";
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use default method in the interface instead of overriding with dummy value.

Comment on lines +21 to +22
<th>Namespace:</th>
<td>{{$ctrl.jmx.Namespace}}</td>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be displayed conditionally, only if not empty. Only HA setup has service ID.

Comment on lines 218 to 224
String localScmServiceId = conf.getTrimmed(
ScmConfigKeys.OZONE_SCM_DEFAULT_SERVICE_ID);
if (localScmServiceId == null) {
Collection<String> scmServiceIds = conf.getTrimmedStringCollection(OZONE_SCM_SERVICE_IDS_KEY);
return String.join(",", scmServiceIds);
}
return localScmServiceId;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call existing method to determine the single service that the datanode belongs to:

public static String getScmServiceId(ConfigurationSource conf) {
String localScmServiceId = conf.getTrimmed(
ScmConfigKeys.OZONE_SCM_DEFAULT_SERVICE_ID);
Collection<String> scmServiceIds;
if (localScmServiceId == null) {
// There is no default scm service id is being set, fall back to ozone
// .scm.service.ids.
scmServiceIds = conf.getTrimmedStringCollection(
OZONE_SCM_SERVICE_IDS_KEY);
if (scmServiceIds.size() > 1) {
throw new ConfigurationException("When multiple SCM Service Ids are " +
"configured," + OZONE_SCM_DEFAULT_SERVICE_ID + " need to be " +
"defined");
} else if (scmServiceIds.size() == 1) {
localScmServiceId = scmServiceIds.iterator().next();
}
}
return localScmServiceId;

to avoid duplication and mismatch (in case of change).

@adoroszlai adoroszlai added the UI label Oct 17, 2024
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @aierate for updating the patch.

@@ -33,6 +33,15 @@
*/
public interface ServiceRuntimeInfo {

/**
* Gets the namespace of Hadoop.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: reference to Hadoop in existing comments is outdated; no need to fix that, but let's not repeat it. :)

@@ -73,6 +74,7 @@

import static org.apache.hadoop.hdds.protocol.DatanodeDetails.Port.Name.HTTP;
import static org.apache.hadoop.hdds.protocol.DatanodeDetails.Port.Name.HTTPS;
import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_SERVICE_IDS_KEY;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsDatanodeService.java
 77: Unused import - org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_SERVICE_IDS_KEY.

Copy link
Contributor Author

@aierate aierate Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adoroszlai Thank you for your very good suggestion. I have fixed these issues, please help me review again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants