HIVE-28749: The default hikaricp.leakDetectionThreshold is not valid #5639
Conversation
|
| @@ -55,6 +56,7 @@ public DataSource create(Configuration hdpConfig, int maxPoolSize) throws SQLExc | |||
| Properties properties = replacePrefix( | |||
| DataSourceProvider.getPrefixedProperties(hdpConfig, HIKARI)); | |||
| long connectionTimeout = hdpConfig.getLong(CONNECTION_TIMEOUT_PROPERTY, 30000L); | |||
| long maxLifetime = hdpConfig.getLong(MAX_LIFETIME, 3600000L); | |||
There was a problem hiding this comment.
what if the max lifetime is equal to the leak detection, will the rule for leak detection not take effect as the max lifetime takes first?
To me, the default of LEAK_DETECTION_THRESHOLD is too long, we can turn down this value, what do you think?
There was a problem hiding this comment.
If we add MAX_LIFETIME then we are adding a new config toggle that was not available before. I am not against doing it but it seems like independent of the main purpose of the PR.
Regarding the value of the threshold I agree with @dengzhhu653 that 1h is probably too long and it makes sense to lower it. In fact, since it is a debug property maybe we should keep it disabled by default (i.e., 0). I don't know how easy it is to pick a value that is useful in every use-case of the metastore.
What changes were proposed in this pull request?
HicariConfig's leak detection threshold has to be smaller than the max lifetime. Our default leak detection threshold is greater than the maximum lifetime defined on the HikariCP side and is disabled by default. Please check the JIRA for details.
https://issues.apache.org/jira/browse/HIVE-28749
Why are the changes needed?
Make our default value effective.
Does this PR introduce any user-facing change?
No.
Is the change a dependency upgrade?
No.
How was this patch tested?
hikaricp.maxLifetimewas configurable in my local machine