Skip to content

[GLUTEN-8479][CORE][Part-2] All configurations should be defined through ConfigEntry#8559

Merged
zhztheplayer merged 1 commit intoapache:mainfrom
yikf:remove-config-val
Jan 21, 2025
Merged

[GLUTEN-8479][CORE][Part-2] All configurations should be defined through ConfigEntry#8559
zhztheplayer merged 1 commit intoapache:mainfrom
yikf:remove-config-val

Conversation

@yikf
Copy link
Copy Markdown
Contributor

@yikf yikf commented Jan 17, 2025

What changes were proposed in this pull request?

Currently, there are three styles for defining Gluten configuration:

  1. Defining keys and default values through variables
  2. Defining through ConfigEntry
  3. Defining through ConfigEntry but referencing variable keys and default values

This PR aims to uniformly use ConfigEntry to define all configurations. Additionally, for internal-use keys (such as spark.gluten.ugi.username), use ReservedKeys to definition.

How was this patch tested?

GA.

@github-actions
Copy link
Copy Markdown

#8479

@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

@yikf
Copy link
Copy Markdown
Contributor Author

yikf commented Jan 17, 2025

@jackylee-ch would you please take a look at this PR?

@yikf yikf force-pushed the remove-config-val branch from 8d77e22 to 273941f Compare January 17, 2025 09:38
@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

@yikf yikf force-pushed the remove-config-val branch from 273941f to 6bef27b Compare January 20, 2025 03:18
@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

@zhztheplayer zhztheplayer changed the title [GLUTEN-8479][CORE][Part-2] All configurations should be defined through ConfigEntry. [GLUTEN-8479][CORE][Part-2] All configurations should be defined through ConfigEntry Jan 20, 2025

/**
* The ReservedKeys retains the configuration constants used internally by Gluten, which are not
* exposed to users.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: Add a TODO to mv other constant keys to this class.

Copy link
Copy Markdown
Contributor

@jackylee-ch jackylee-ch left a comment

Choose a reason for hiding this comment

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

@yikf yikf force-pushed the remove-config-val branch from 6bef27b to ef7582b Compare January 20, 2025 12:31
@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

Copy link
Copy Markdown
Contributor

@Yohahaha Yohahaha left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown
Member

@zhztheplayer zhztheplayer left a comment

Choose a reason for hiding this comment

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

This is a solid code cleanup. Thank you very much for the contribution.

@zhztheplayer zhztheplayer merged commit df66e93 into apache:main Jan 21, 2025
@yikf yikf deleted the remove-config-val branch January 21, 2025 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants