Check config value in numeric or duration range #5228
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #5228 +/- ##
=========================================
Coverage 61.41% 61.42%
Complexity 23 23
=========================================
Files 607 608 +1
Lines 35948 36000 +52
Branches 4939 4951 +12
=========================================
+ Hits 22078 22112 +34
- Misses 11484 11500 +16
- Partials 2386 2388 +2 ☔ View full report in Codecov by Sentry. |
bowenliang123
force-pushed
the
config-checkvalue
branch
from
595d61a to
a79babf
Compare
| .createWithDefault(Duration.ofMinutes(1).toMillis) | ||
|
|
||
| val CREDENTIALS_CHECK_INTERVAL: ConfigEntry[Long] = | ||
| buildConf("kyuubi.credentials.check.interval") | ||
| .doc("The interval to check the expiration of cached <user, CredentialsRef> pairs.") | ||
| .version("1.6.0") | ||
| .timeConf | ||
| .checkValue(_ > Duration.ofSeconds(3).toMillis, "Minimum 3 seconds") | ||
| .checkDurationRange(min = Some(Duration.ofSeconds(3))) |
There was a problem hiding this comment.
_ > should have different semantics from checkDurationRange.
There was a problem hiding this comment.
It's very likely an original typing mistake from #1021. The 'Minimum' term in the error message hints allowing >= inclusively rather than > exclusively as well. And there's no technical issue to exclude the start value in this config. Also taking Scala's numeric Range class as an example, the start value is always inclusive.
cc @lightning-L
There was a problem hiding this comment.
Oh....
I am okay with the change. Thanks
bowenliang123
force-pushed
the
config-checkvalue
branch
from
c2b82e8 to
1432251
Compare
|
Do you have some time to have a look at this guarding checks for config values? If we docide not to accept it , let me close this PR. @pan3793 |
pan3793
changed the title
kyuubi-common/src/main/scala/org/apache/kyuubi/config/ConfigBuilder.scala
Outdated
Show resolved
Hide resolved
kyuubi-util-scala/src/main/scala/org/apache/kyuubi/util/NumUtils.scala
Outdated
Show resolved
Hide resolved
kyuubi-util-scala/src/main/scala/org/apache/kyuubi/util/NumUtils.scala
Outdated
Show resolved
Hide resolved
bowenliang123
force-pushed
the
config-checkvalue
branch
from
1432251 to
6fcfcae
Compare
kyuubi-util-scala/src/main/scala/org/apache/kyuubi/util/MathUtils.scala
Outdated
Show resolved
Hide resolved
bowenliang123
force-pushed
the
config-checkvalue
branch
from
6ea7b01 to
cd04374
Compare
| min: Option[Duration] = None, | ||
| max: Option[Duration] = None, |
There was a problem hiding this comment.
Do you think we should change the type from : Option[Duration] = None into : Duration = null for simplified usage ? @pan3793
There was a problem hiding this comment.
I prefer None, but force-inclusive is not flexible
There was a problem hiding this comment.
So how about using the current implementation first?
kyuubi-util-scala/src/main/scala/org/apache/kyuubi/util/MathUtils.scala
Outdated
Show resolved
Hide resolved
bowenliang123
force-pushed
the
config-checkvalue
branch
from
a7a5f4b to
6995020
Compare
|
Closing this PR with no enough consensus on the purposes, the design, the changes and the approaches. |
Why are the changes needed?
checkPositivecheckNonnegativecheckRangeby NumericRangecheckPatternfor string typecheckDurationby min/max range for duration typeHow 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 make a pull request
Was this patch authored or co-authored using generative AI tooling?
No.