(arrow) branch main updated: GH-46217: [C++][Parquet] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctly (#46283)

[apitrou]

This is an automated email from the ASF dual-hosted git repository.

apitrou pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git


The following commit(s) were added to refs/heads/main by this push:
     new eda9942a4d GH-46217: [C++][Parquet] Update the timestamp of 
parquet::encryption::TwoLevelCacheWithExpiration correctly (#46283)
eda9942a4d is described below

commit eda9942a4d6e78f9017d928ce39346ec2c13d03c
Author: Eddie Chang <kalcifer7...@gmail.com>
AuthorDate: Tue Jul 1 23:24:37 2025 +0800

    GH-46217: [C++][Parquet] Update the timestamp of 
parquet::encryption::TwoLevelCacheWithExpiration correctly (#46283)
    
    ### Rationale for this change
    
    Fix the bug mentioned in 
https://github.com/apache/arrow/issues/46217#issuecomment-2825621423
    
    ### What changes are included in this PR?
    
    * Fix the logic of timestamp modification.
    * Extend the test in two_level_cache_with_expiration_test.cc 
(CleanupPeriodOk).
    * Replace the usage of std::make_unique's constructor with 
std::make_unique().
    
    ### Are these changes tested?
    
    Yes. The original test didn't expose the problem because 
CheckCacheForExpiredTokens was only called once.
    
    ### Are there any user-facing changes?
    
    No.
    * GitHub Issue: #46217
    
    Lead-authored-by: Eddie Chang <kalcifer7...@gmail.com>
    Co-authored-by: Sutou Kouhei <k...@cozmixng.org>
    Signed-off-by: Antoine Pitrou <pit...@free.fr>
---
 .../parquet/encryption/two_level_cache_with_expiration.h   | 14 +++-----------
 .../encryption/two_level_cache_with_expiration_test.cc     | 10 +++++++++-
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/cpp/src/parquet/encryption/two_level_cache_with_expiration.h 
b/cpp/src/parquet/encryption/two_level_cache_with_expiration.h
index 76c2b82770..283ebd97b7 100644
--- a/cpp/src/parquet/encryption/two_level_cache_with_expiration.h
+++ b/cpp/src/parquet/encryption/two_level_cache_with_expiration.h
@@ -103,32 +103,24 @@ class TwoLevelCacheWithExpiration {
     if (external_cache_entry == cache_.end() ||
         external_cache_entry->second.IsExpired()) {
       cache_.insert({access_token, internal::ExpiringCacheMapEntry<V>(
-                                       
std::shared_ptr<ConcurrentMap<std::string, V>>(
-                                           new ConcurrentMap<std::string, 
V>()),
+                                       
std::make_shared<ConcurrentMap<std::string, V>>(),
                                        cache_entry_lifetime_seconds)});
     }
 
     return cache_[access_token].cached_item();
   }
 
-  void CheckCacheForExpiredTokens(double cache_cleanup_period_seconds) {
+  void CheckCacheForExpiredTokens(double cache_cleanup_period_seconds = 0.0) {
     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);
+      last_cache_cleanup_timestamp_ = now;
     }
   }
 
-  void RemoveExpiredEntriesFromCache() {
-    auto lock = mutex_.Lock();
-
-    RemoveExpiredEntriesNoMutex();
-  }
-
   void Remove(const std::string& access_token) {
     auto lock = mutex_.Lock();
     cache_.erase(access_token);
diff --git a/cpp/src/parquet/encryption/two_level_cache_with_expiration_test.cc 
b/cpp/src/parquet/encryption/two_level_cache_with_expiration_test.cc
index d8f2c62551..75c31a907c 100644
--- a/cpp/src/parquet/encryption/two_level_cache_with_expiration_test.cc
+++ b/cpp/src/parquet/encryption/two_level_cache_with_expiration_test.cc
@@ -77,7 +77,7 @@ TEST_F(TwoLevelCacheWithExpirationTest, RemoveExpiration) {
   // lifetime2 will not be expired
   SleepFor(0.3);
   // now clear expired items from the cache
-  cache_.RemoveExpiredEntriesFromCache();
+  cache_.CheckCacheForExpiredTokens();
 
   // lifetime1 (with 2 items) is expired and has been removed from the cache.
   // Now the cache create a new object which has no item.
@@ -114,6 +114,14 @@ TEST_F(TwoLevelCacheWithExpirationTest, CleanupPeriodOk) {
   // lifetime2 is not expired and still contains 2 items.
   auto lifetime2 = cache_.GetOrCreateInternalCache("lifetime2", 3);
   ASSERT_EQ(lifetime2->size(), 2);
+
+  // The further process is added to test whether the timestamp is set 
correctly.
+  // CheckCacheForExpiredTokens() should be called at least twice to verify the
+  // correctness.
+  SleepFor(0.3);
+  cache_.CheckCacheForExpiredTokens(0.2);
+  lifetime2 = cache_.GetOrCreateInternalCache("lifetime2", 0.5);
+  ASSERT_EQ(lifetime2->size(), 0);
 }
 
 TEST_F(TwoLevelCacheWithExpirationTest, RemoveByToken) {