From cfce2437d03fc28bc71460db8378445c422245a0 Mon Sep 17 00:00:00 2001 From: Eddie Chang Date: Thu, 24 Apr 2025 12:57:23 +0800 Subject: [PATCH 1/4] Update the timestamp of cache correctly --- .../parquet/encryption/two_level_cache_with_expiration.h | 3 +-- .../encryption/two_level_cache_with_expiration_test.cc | 8 ++++++++ 2 files changed, 9 insertions(+), 2 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 76c2b827700..403558cfcab 100644 --- a/cpp/src/parquet/encryption/two_level_cache_with_expiration.h +++ b/cpp/src/parquet/encryption/two_level_cache_with_expiration.h @@ -118,8 +118,7 @@ class TwoLevelCacheWithExpiration { if (now > (last_cache_cleanup_timestamp_ + std::chrono::duration(cache_cleanup_period_seconds))) { RemoveExpiredEntriesNoMutex(); - last_cache_cleanup_timestamp_ = - now + std::chrono::duration(cache_cleanup_period_seconds); + last_cache_cleanup_timestamp_ = now; } } 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 d8f2c625514..22e59872091 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 @@ -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. + // CheckCheckCacheForExpiredTokens() 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) { From 69ab6b9f58fb4c59b44203e301d8f887fd099b6a Mon Sep 17 00:00:00 2001 From: Eddie Chang Date: Thu, 1 May 2025 16:41:15 +0000 Subject: [PATCH 2/4] Replace std::shared_ptr's constructor with std::make_shared --- cpp/src/parquet/encryption/two_level_cache_with_expiration.h | 3 +-- 1 file changed, 1 insertion(+), 2 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 403558cfcab..c7688df0f04 100644 --- a/cpp/src/parquet/encryption/two_level_cache_with_expiration.h +++ b/cpp/src/parquet/encryption/two_level_cache_with_expiration.h @@ -103,8 +103,7 @@ class TwoLevelCacheWithExpiration { if (external_cache_entry == cache_.end() || external_cache_entry->second.IsExpired()) { cache_.insert({access_token, internal::ExpiringCacheMapEntry( - std::shared_ptr>( - new ConcurrentMap()), + std::make_shared>(), cache_entry_lifetime_seconds)}); } From 463e042c7e276ffadf62d8e61b0819f1191ead18 Mon Sep 17 00:00:00 2001 From: Eddie Chang Date: Fri, 2 May 2025 08:18:57 +0800 Subject: [PATCH 3/4] Fix typo Co-authored-by: Sutou Kouhei --- .../parquet/encryption/two_level_cache_with_expiration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 22e59872091..4b2d16f6281 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 @@ -116,7 +116,7 @@ TEST_F(TwoLevelCacheWithExpirationTest, CleanupPeriodOk) { ASSERT_EQ(lifetime2->size(), 2); // The further process is added to test whether the timestamp is set correctly. - // CheckCheckCacheForExpiredTokens() should be called at least twice to verify the + // CheckCacheForExpiredTokens() should be called at least twice to verify the // correctness. SleepFor(0.3); cache_.CheckCacheForExpiredTokens(0.2); From e23052bb40a145d3487de347c193e30371c8ec02 Mon Sep 17 00:00:00 2001 From: Eddie Chang Date: Fri, 2 May 2025 08:14:41 +0000 Subject: [PATCH 4/4] Remove the duplicate code from TwoLevelCacheWithExpiration * RemoveExpiredEntriesFromCache didn't update the timestamp while it should. If it did, it would literally be CheckCacheForExpiredTokens(0.0). I've added a default value to CheckCacheForExpiredTokens to make it more natural and removed RemoveExpiredEntriesFromCache. * Update the call sites accordingly. --- .../parquet/encryption/two_level_cache_with_expiration.h | 8 +------- .../encryption/two_level_cache_with_expiration_test.cc | 2 +- 2 files changed, 2 insertions(+), 8 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 c7688df0f04..283ebd97b71 100644 --- a/cpp/src/parquet/encryption/two_level_cache_with_expiration.h +++ b/cpp/src/parquet/encryption/two_level_cache_with_expiration.h @@ -110,7 +110,7 @@ class TwoLevelCacheWithExpiration { 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(); @@ -121,12 +121,6 @@ class TwoLevelCacheWithExpiration { } } - 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 4b2d16f6281..75c31a907c0 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.