Skip to content

Conversation

@kmizumar
Copy link
Contributor

@kmizumar kmizumar commented Mar 1, 2023

What changes were proposed in this pull request?

Add DATASTREAM tag as a supported one described in the ozone-default.xml comment.

What is the link to the Apache JIRA

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

How was this patch tested?

No tests are required. Just update a comment.

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 @kmizumar for the patch.

This list seems to be out of date. Not only was DATASTREAM missing, but there are several other differences when compared to the list of tags Ozone actually has in the configs.

Also, there are two related config properties (ultimately used by HddsConfServlet), which also have mismatches:

<property>
<name>hadoop.tags.custom</name>
<value>OZONE,MANAGEMENT,SECURITY,PERFORMANCE,DEBUG,CLIENT,SERVER,OM,SCM,
CRITICAL,RATIS,CONTAINER,REQUIRED,REST,STORAGE,PIPELINE,STANDALONE,S3GATEWAY,RECON</value>
</property>
<property>
<name>ozone.tags.system</name>
<value>OZONE,MANAGEMENT,SECURITY,PERFORMANCE,DEBUG,CLIENT,SERVER,OM,SCM,
CRITICAL,RATIS,CONTAINER,REQUIRED,REST,STORAGE,PIPELINE,STANDALONE,S3GATEWAY,TOKEN,TLS,RECON</value>
</property>

I have compiled a list of tags from different sources:

(Hopefully I got those right.)

Would you like to fix these discrepancies as well?

@adoroszlai
Copy link
Contributor

Note: #4328 is also changing these two properties (removing whitespace).

@kmizumar
Copy link
Contributor Author

kmizumar commented Mar 1, 2023

Thanks for the thorough list of tags, @adoroszlai .
Then, how about adding a list of known tags as a comment in ozone-default.xml like this?

@adoroszlai
Copy link
Contributor

Then, how about adding a list of known tags as a comment in ozone-default.xml like this?

I think it would be more useful to:

  1. Update hadoop.tags.custom and ozone.tags.system to reflect the list of tags actually in use (combined from XML and annotated classes, i.e. used.txt). My understanding is that the both properties should have the same value.
  2. Remove the list of tags from the comment, and provide some instructions instead, something like:
    * List of existing tags is defined by the properties `hadoop.tags.custom` and `ozone.tags.system`.
    * If you start using a new tag, please update both lists.
    

@kerneltime
Copy link
Contributor

@SaketaChalamchala can you please take a look?

@SaketaChalamchala
Copy link
Contributor

Agree with @adoroszlai

  1. Update hadoop.tags.custom and ozone.tags.system to reflect the list of tags actually in use (combined from XML and annotated classes, i.e. used.txt). My understanding is that the both properties should have the same value.

hadoop.tags.custom is being used to build a propertyTagsMap that is used to search properties by available tags. Tags in ozone.tags.system are being used to populate configs in SCM and OM UI configuration page. So, it would be better for both of these properties to have all tags in use.

Another note here, it would be cleaner if we replace all DN tags with DATANODE and S3G tags with S3GATEWAY in ozone-default.xml. That way the full list of tags would be ACL,BALANCER,CLIENT,CONTAINER,CRITICAL,DATANODE,DATASTREAM,DEBUG,DELETION,DEPRECATED,FREON,HA,HDDS,KERBEROS,MANAGEMENT,OM,OPERATION,OZONE,OZONEFS,PERFORMANCE,PIPELINE,RATIS,RECON,REQUIRED,REST,S3GATEWAY,SCM,SECURITY,SERVER,STANDALONE,STORAGE,TLS,TOKEN,UPGRADE,X509. Please feel free to validate this list.

2. Remove the list of tags from the comment, and provide some instructions instead, something like:
* List of existing tags is defined by the properties `hadoop.tags.custom` and `ozone.tags.system`. * If you start using a new tag, please update both lists.

One more note, tags in enum in ConfigTag.java are being used in @Config annotations.
So, it might also be useful to also add another instruction here.

* Note: the values are defined in ozone-default.xml by hadoop.tags.custom and ozone.tags.system.
* If you start using a new tag, please update both lists in ozone-default.xml as well.

@SaketaChalamchala
Copy link
Contributor

@kmizumar you can change the PR title to HDDS-8048. DATASTREAM is not listed as supported tag in ozone-default.xml to pass the pull request / title check

@adoroszlai
Copy link
Contributor

Thanks @SaketaChalamchala for your comment. Now that you have mentioned the ConfigTag enum, I just realized that we could eliminate the duplication and possible discrepancies by making ConfigTag the definitive source. See #4359 for my proposal.

@adoroszlai
Copy link
Contributor

Thanks again @kmizumar for reporting the problem. #4359 has replaced the obsolete list with a reference to the ConfigTag enum, and made the latter the definitive source of truth about tags.

@adoroszlai adoroszlai closed this Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants