You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fluss.apache.org by "fhan688 (via GitHub)" <gi...@apache.org> on 2026/09/29 08:29:12 UTC
[PR] [common] Make boolean config parsing locale-independent [fluss]
fhan688 opened a new pull request, #4515:
URL: https://github.com/apache/fluss/pull/4515
### Purpose
`ConfigurationUtils.convertToBoolean` uses `String.toUpperCase()` without an explicit locale. This makes configuration normalization depend on the JVM's default locale.
Configuration parsing should behave consistently across different runtime environments.
### Brief change log
- Use `Locale.ROOT` when normalizing boolean configuration values.
- Add a test that parses mixed-case boolean values with the Turkish locale configured as the JVM default.
- Restore the original default locale after the test to avoid affecting other tests.
### Tests
The following test passed:
```text
ConfigurationTest
A total of 25 test cases passed with no failures or errors.
Checkstyle, Spotless, and RAT checks also passed.
```
### API and Format
No public API or storage format changes.
### Documentation
No documentation changes are required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Make boolean config parsing locale-independent [fluss]
Posted by "utafrali (via GitHub)" <gi...@apache.org>.
utafrali commented on code in PR #4515:
URL: https://github.com/apache/fluss/pull/4515#discussion_r4131289482
##########
fluss-common/src/test/java/org/apache/fluss/config/ConfigurationTest.java:
##########
@@ -137,6 +137,23 @@ void testGetterAndSetter() throws Exception {
assertThat(conf.get(MEMORY_OPTION).getBytes()).isEqualTo(65536L);
}
+ @Test
+ void testBooleanParsingIsLocaleIndependent() {
+ Locale originalLocale = Locale.getDefault();
+ try {
+ Locale.setDefault(Locale.forLanguageTag("tr-TR"));
Review Comment:
The test sets the Turkish locale to demonstrate locale-dependent breakage, but none of the characters in "true" or "false" (t, r, u, e, f, a, l, s) are affected by Turkish case-mapping rules. In Turkish, the special casing only applies to 'i' -> 'İ' and 'I' -> 'ı'. This means the test passes with or without the fix, so it does not actually verify the regression scenario claimed in the PR description.
The most honest path forward is to acknowledge this constraint with an explicit comment:
```java
// Note: "true" and "false" contain no characters with locale-sensitive casing
// (Turkish's special 'i'/'I' rules don't apply here). This test documents intent
// and guards against future inputs that might contain such characters.
Locale.setDefault(Locale.forLanguageTag("tr-TR"));
```
Alternatively, test `ConfigurationUtils.convertToBoolean` directly with a locale-sensitive input, or drop the locale machinery and simply unit-test that `convertToBoolean` accepts all mixed-case variants (`"TrUe"`, `"TRUE"`, `"false"`, etc.).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Make boolean config parsing locale-independent [fluss]
Posted by "luoyuxia (via GitHub)" <gi...@apache.org>.
luoyuxia merged PR #4515:
URL: https://github.com/apache/fluss/pull/4515
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Make boolean config parsing locale-independent [fluss]
Posted by "fhan688 (via GitHub)" <gi...@apache.org>.
fhan688 commented on code in PR #4515:
URL: https://github.com/apache/fluss/pull/4515#discussion_r4131436815
##########
fluss-common/src/test/java/org/apache/fluss/config/ConfigurationTest.java:
##########
@@ -137,6 +137,23 @@ void testGetterAndSetter() throws Exception {
assertThat(conf.get(MEMORY_OPTION).getBytes()).isEqualTo(65536L);
}
+ @Test
+ void testBooleanParsingIsLocaleIndependent() {
+ Locale originalLocale = Locale.getDefault();
+ try {
+ Locale.setDefault(Locale.forLanguageTag("tr-TR"));
Review Comment:
Thanks for pointing this out. You're right that neither "true" nor "false" contains locale-sensitive characters, so the Turkish-locale test passes both before and after this change and does not demonstrate a regression.
I've removed the test. The production change is retained as deterministic hardening and to keep boolean normalization consistent with the existing Locale.ROOT usage in ConfigurationUtils. I've also updated the PR description to clarify that the currently accepted boolean values are not affected by locale-specific case mappings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Make boolean config parsing locale-independent [fluss]
Posted by "luoyuxia (via GitHub)" <gi...@apache.org>.
luoyuxia commented on PR #4515:
URL: https://github.com/apache/fluss/pull/4515#issuecomment-5891073708
@coding-otter-bot review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [common] Make boolean config parsing locale-independent [fluss]
Posted by "coding-otter-bot (via GitHub)" <gi...@apache.org>.
coding-otter-bot commented on PR #4515:
URL: https://github.com/apache/fluss/pull/4515#issuecomment-5891138009
## Automated code review
### Problem
ConfigurationUtils.convertToBoolean normalized string values with locale-sensitive String.toUpperCase(), making boolean config parsing depend on the JVM default locale and inconsistent with other locale-neutral normalization in ConfigurationUtils.
### Approach
The changed expression now calls toUpperCase(Locale.ROOT). java.util.Locale was already imported and used elsewhere in ConfigurationUtils, so the change is a one-line normalization update with no new API or import requirement.
### Review coverage
Complete: the material changed paths and review hypotheses were resolved.
### Conclusion
No actionable code issues were found in this static review. From the reviewed code paths, no change is required before merge. CI results and maintainer approval still apply.
### Findings
No actionable findings survived the final audit.
<!-- fluss-agent-publication:dc7223e5c83cac1f77d292831a23f0858c214fe0f67f96ee57076e31443af13c -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org