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.

Sign up for GitHub

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

Jump to bottom

[Enhancement] Support persistent configuration #54941

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

[Enhancement] Support persistent configuration #54941

wants to merge 21 commits into from

Conversation

crossoverJie
Copy link
Contributor

Why I'm doing:

What I'm doing:

Fixes #52374

Using ADMIN SET FRONTEND CONFIG ("key" = "value") WITH PERSISTENT; to persist the configuration.
When restarting FE, these configurations still take effect.

This feature is only effective when the configuration file has write permissions in a virtual machine environment (it cannot be used in a container environment).

What type of PR is this:

  • BugFix
  • Feature
  • Enhancement
  • Refactor
  • UT
  • Doc
  • Tool

Does this PR entail a change in behavior?

  • Yes, this PR will result in a change in behavior.
  • No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

  • Interface/UI changes: syntax, type conversion, expression evaluation, display information
  • Parameter changes: default values, similar parameters but with different default values
  • Policy changes: use new policy to replace old one, functionality automatically enabled
  • Feature removed
  • Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

  • I have added test cases for my bug fix or my new feature
  • This pr needs user documentation (for new or modified features or behaviors)
    • I have added documentation for my new feature or new function
  • This is a backport pr

Bugfix cherry-pick branch check:

  • I have checked the version labels which the pr will be auto-backported to the target branch
    • 3.4
    • 3.3
    • 3.2
    • 3.1
    • 3.0

@crossoverJie
config persistent v2
212d7f3 
Signed-off-by: crossoverJie <[email protected]>
@crossoverJie crossoverJie requested review from a team as code owners January 10, 2025 08:24
@wanpengfei-git wanpengfei-git requested a review from a team January 10, 2025 08:24
@crossoverJie
fix checkstyle
8ec60a1 
Signed-off-by: crossoverJie <[email protected]>
@crossoverJie
trigger ci
f0b0582 
Signed-off-by: crossoverJie <[email protected]>
@crossoverJie
trigger ci
5a31417 
Signed-off-by: crossoverJie <[email protected]>
@crossoverJie
trigger ci
52d17cc 
Signed-off-by: crossoverJie <[email protected]>
@crossoverJie
trigger ci
cfe11e3 
Signed-off-by: crossoverJie <[email protected]>
@crossoverJie
trigger ci
abd736c 
Signed-off-by: crossoverJie <[email protected]>
@crossoverJie
trigger ci
1173a9c 
Signed-off-by: crossoverJie <[email protected]>
@crossoverJie
trigger ci
43f4b1b 
Signed-off-by: crossoverJie <[email protected]>
@crossoverJie
trigger ci
5872b99 
Signed-off-by: crossoverJie <[email protected]>
@crossoverJie
trigger ci
c9a02dd 
Signed-off-by: crossoverJie <[email protected]>
@crossoverJie
trigger ci
6906d1b 
Signed-off-by: crossoverJie <[email protected]>
@Samrose-Ahmed
Copy link
Contributor

Perhaps you can explain your approach in case I don't understand, I dont think this is a good approach to check if in container and writing to files. Can't we use a different approach like persisiting to Starrocks meta?

@crossoverJie
trigger ci
51f5d8c 
Signed-off-by: crossoverJie <[email protected]>
@crossoverJie
Copy link
Contributor Author

Perhaps you can explain your approach in case I don't understand, I dont think this is a good approach to check if in container and writing to files. Can't we use a different approach like persisiting to Starrocks meta?

Sorry for not explaining the background clearly.

In the k8s environment, users maintain configuration files using configmap, and each upgrade causes the configuration to be overwritten. Therefore, it has been decided to temporarily support configuration file persistence only in virtual machine environments.

The above background has been agreed upon with @kevincai @gengjun-git .

gengjun-git
gengjun-git reviewed Jan 20, 2025
View reviewed changes
fe/fe-core/src/main/java/com/starrocks/common/ConfigBase.java Outdated
try (FileReader reader = new FileReader(configPath)) {
props.load(reader);
}
String oldValue = props.getProperty(key);
Copy link
Contributor

Choose a reason for hiding this comment

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

if the oldValue is null, the comment should be:

The user xxx added key = value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

fe/fe-core/src/main/java/com/starrocks/common/ConfigBase.java Outdated
writer.write(key + " = " + value);
writer.newLine();

while (i + 1 < lines.size() && !lines.get(i + 1).startsWith("#") && !lines.get(i + 1).contains("=")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove the following comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's been fixed.

@crossoverJie
revert
a68f048 
Signed-off-by: crossoverJie <[email protected]>
@crossoverJie
Using FORWARD_NO_SYNC Simplify Code
6562672 
Signed-off-by: crossoverJie <[email protected]>
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
13 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@github-actions GitHub Actions
Copy link

[BE Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

@github-actions GitHub Actions
Copy link

[Java-Extensions Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

@github-actions GitHub Actions
Copy link

[FE Incremental Coverage Report]

❌ fail : 52 / 78 (66.67%)

file detail

path covered_line new_line coverage not_covered_line_detail
🔵 com/starrocks/server/NodeMgr.java 5 26 19.23% [1165, 1166, 1168, 1173, 1174, 1175, 1176, 1177, 1179, 1181, 1183, 1184, 1185, 1186, 1187, 1188, 1190, 1192, 1193, 1194, 1196]
🔵 com/starrocks/common/ConfigBase.java 41 46 89.13% [324, 325, 326, 331, 332]
🔵 com/starrocks/sql/ast/AdminSetConfigStmt.java 3 3 100.00% []
🔵 com/starrocks/service/FrontendServiceImpl.java 2 2 100.00% []
🔵 com/starrocks/common/util/Util.java 1 1 100.00% []

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

@gengjun-git gengjun-git gengjun-git approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees

@crossoverJie crossoverJie

Labels
behavior_changed PROTO-REVIEW
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support persistent configuration
4 participants
@crossoverJie @Samrose-Ahmed @gengjun-git @wanpengfei-git