[AMORO-3418] Optimize ams configuration to support parsing of time and storage unit #3423
Conversation
klion26
force-pushed
the
optimize_ams_configuration
branch
from
53ed46f to
d6efb5a
Compare
amoro-ams/src/main/java/org/apache/amoro/server/AmoroManagementConf.java
Outdated
Show resolved
Hide resolved
| .longType() | ||
| .defaultValue(30000L) | ||
| .durationType() | ||
| .defaultValue(Duration.ofSeconds(30)) |
There was a problem hiding this comment.
If a user sets this conf to 60000 before this commit, and he/she needs to adjust the config to 60, am I understanding right? if it is true, can we still use millisecond as the unit in the current commit?
There was a problem hiding this comment.
Not exactly, the default unit for all time-related configuration items is currently milliseconds, which is consistent with the pre-commit default unit, except for the configuration item terminal.session.timeout. For terminal.session.timeout, the default unit is minutes before this commit and milliseconds after the commit. I think this change should be highlighted in the release logs or upgrade scripts indeed.
| import java.util.Map; | ||
|
|
||
| public class TestAmoroManagementConf { | ||
| private static final ConfigOption<Duration>[] TIME_RELATED_CONFIG_OPTIONS = |
There was a problem hiding this comment.
Do we need to add a test to cover the migration for the changed configs? so that we know this does not change the user behavior, or if the behavior is indeed needed, we need to add them to the release note so that the user can know it(maybe add a upgrade script is better)
There was a problem hiding this comment.
I have added a unit test to parse the old version of the config file in the latest commit. The parsing result is as expected after adding a unit suffix to the original values of the old config file, which might serve as a reference for the upgrade scripts.
…terval and storage related configuration items when both values and units are specified
…time interval and storage related configuration items when both values and units are specified
Jzjsnow
force-pushed
the
optimize_ams_configuration
branch
from
d6efb5a to
bf94632
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3423 +/- ##
============================================
+ Coverage 21.80% 27.78% +5.97%
- Complexity 2353 3591 +1238
============================================
Files 431 602 +171
Lines 39948 48678 +8730
Branches 5655 6274 +619
============================================
+ Hits 8712 13524 +4812
- Misses 30504 34214 +3710
- Partials 732 940 +208
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…time interval and storage related configuration items when both values and units are specified
Jzjsnow
force-pushed
the
optimize_ams_configuration
branch
from
bf94632 to
1d4337e
Compare
Why are the changes needed?
The time-related configuration items in the current ams configuration are inconsistent in type, with both
LongandDurationtypes. This results in users having to convert to milliseconds when using theseLongtypes, which is not user friendly.In addition, storage-related parameters such as thrift-server.max-message-size only support Long type (default unit is byte). It would be more user-friendly to support user-specified units, as in the Apache Flink configuration.
Close #3418.
Brief change log
Duration.How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before making a pull request
Documentation