kapoisu opened a new issue, #46217:
URL: https://github.com/apache/arrow/issues/46217
### Describe the enhancement requested
KeyToolkit is used internally according to the comment. It looks more like a
mere aggregate. I suggest redefine it as a struct and change existing member
functions to free functions, which can be implemented by the public interfaces
of existing member variables. The only exception which may introduce invariants
is the first part of KeyToolket::RotateMasterKeys(),
```c++
const auto now = internal::CurrentTimePoint();
auto lock = last_cache_clean_for_key_rotation_time_mutex_.Lock();
if (now > last_cache_clean_for_key_rotation_time_ +
std::chrono::duration<double>(kCacheCleanPeriodForKeyRotation)) {
kek_write_cache_per_token().Clear();
last_cache_clean_for_key_rotation_time_ = now;
}
lock.Unlock();
```
However, TwoLevelCacheWithExpiration (the object return by
kek_write_cache_per_token() in the code above) has nearly identical logic,
```c++
void CheckCacheForExpiredTokens(double cache_cleanup_period_seconds) {
auto lock = mutex_.Lock();
const auto now = internal::CurrentTimePoint();
if (now > (last_cache_cleanup_timestamp_ +
std::chrono::duration<double>(cache_cleanup_period_seconds)))
{
RemoveExpiredEntriesNoMutex();
last_cache_cleanup_timestamp_ =
now + std::chrono::duration<double>(cache_cleanup_period_seconds);
}
}
```
except that the timestamp is set to now + cache_cleanup_period_seconds,
which looks like a bug.
The same change can be applied to KeyWithMasterId.
### Component(s)
C++
--
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-unsubscr...@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
