Skip to content

Conversation

@adoroszlai
Copy link
Contributor

@adoroszlai adoroszlai commented Mar 7, 2023

What changes were proposed in this pull request?

Configuration properties can be tagged as a way of grouping (HADOOP-15005). Tags can be used as a filter when browsing config on the web UI.

While any string can be used as tag, the web UI only supports a specific set of tags, defined by ozone.tags.system. To hook into Hadoop config tagging, the same value needs to be set as hadoop.tags.custom. ozone-default.xml also lists config tags in a header comment as a hint.

Ozone also supports type-safe configuration annotated with @Config, which allows tags defined in the enum ConfigTag.

All these result in duplication and discrepancies between the various lists of tags. Web UI has some unused tags, and approximately half of all tags in use are missing:

before

I propose to:

  1. use ConfigTag as the definitive source
  2. remove ozone.tags.system and hadoop.tags.custom from ozone-default.xml
  3. replace the header comment in ozone-default.xml with a reference to ConfigTag

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

How was this patch tested?

Updated TestOzoneConfiguration and TestOzoneConfigurationFields.

Verified config tags exposed in SCM web UI:

after

Regular CI (in progress):
https://github.com/adoroszlai/hadoop-ozone/actions/runs/4352553967

@adoroszlai adoroszlai marked this pull request as draft March 7, 2023 09:59
@adoroszlai adoroszlai self-assigned this Mar 7, 2023
@adoroszlai adoroszlai added the enhancement New feature or request label Mar 7, 2023
@adoroszlai adoroszlai marked this pull request as ready for review March 7, 2023 11:09
public final class OzoneConfigKeys {
public static final String OZONE_TAGS_SYSTEM_KEY =
"ozone.tags.system";
public static final String DFS_CONTAINER_IPC_PORT =
Copy link
Contributor

Choose a reason for hiding this comment

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

For my understanding, Is there a reason why this config needed to be taken out of OzoneConfigKeys and added in OzoneConfiguration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @SaketaChalamchala for the review.

TestOzoneConfigurationFields verifies that all keys defined in OzoneConfigKeys (and some others) are also present in ozone-default.xml (and vice versa). It would have been enough to add as an exception in TestOzoneConfigurationFields.

@SaketaChalamchala
Copy link
Contributor

Thanks for the patch @adoroszlai. The change looks good. Just had a question. See my comment above.
@sumitagrawl and @errose28 could you also take a look at this?

Copy link
Contributor

@fapifta fapifta left a comment

Choose a reason for hiding this comment

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

Thank you for taking this on @adoroszlai.

Actually I think as the OZONE_TAGS_SYSTEM_KEY constant is used internally within the OzoneConfiguration, to work with the tagging system, it has a good place within OzoneConfiguration instead of within the config keys. Especially as we do not really want this to be set from outside after switching to the enum as the source of truth of the available tags.

@adoroszlai adoroszlai merged commit bfc02e4 into apache:master Mar 9, 2023
@adoroszlai adoroszlai deleted the HDDS-8091 branch March 9, 2023 17:56
@adoroszlai
Copy link
Contributor Author

Thanks @fapifta, @SaketaChalamchala for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants