Skip to content

GH-46217: [C++][Parquet] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctly - #46283

Merged
pitrou merged 4 commits into
apache:mainfrom
kapoisu:GH-46217
Jul 1, 2025
Merged

GH-46217: [C++][Parquet] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctly#46283
pitrou merged 4 commits into
apache:mainfrom
kapoisu:GH-46217

Conversation

@kapoisu

@kapoisukapoisu commented May 1, 2025•
edited
Loading

Copy link
Copy Markdown
Contributor

Rationale for this change

Fix the bug mentioned in #46217 (comment)

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.

@kapoisu
kapoisu requested a review from wgtmac as a code ownerMay 1, 2025 17:19
@kapoisukapoisu changed the title GH-46217: Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctlyGH-46217: [C++] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctlyMay 1, 2025
@github-actionsgithub-actionsBot added the awaiting review Awaiting review label May 1, 2025
@github-actions

github-actionsBot commented May 1, 2025

Copy link
Copy Markdown

⚠️ GitHub issue #46217has been automatically assigned in GitHub to PR creator.

kou
kou approved these changes May 2, 2025
View reviewed changes

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Comment threadcpp/src/parquet/encryption/two_level_cache_with_expiration_test.cc Outdated
@github-actionsgithub-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 2, 2025
@kapoisu

kapoisu commented May 2, 2025

Copy link
Copy Markdown
ContributorAuthor

I'll try to see if I can fix the error during CI. It seems similar to #46203.

adamreeve
adamreeve reviewed May 4, 2025
View reviewed changes
Comment threadcpp/src/parquet/encryption/two_level_cache_with_expiration.h Outdated
@kapoisu

kapoisu commented May 6, 2025

Copy link
Copy Markdown
ContributorAuthor

The mismatch of schemas during integration tests seems related to #46087. Since the issue was closed a few days ago (via #46110), I rebased my branch to see if the previously failed tests would pass now.

@wgtmacwgtmac changed the title GH-46217: [C++] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctlyGH-46217: [C++][Parquet] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctlyMay 6, 2025
@pitrou

pitrou commented Jun 5, 2025

Copy link
Copy Markdown
Member

I've rebased on git main and will merge if CI is ok.

kapoisuand others added 4 commits July 1, 2025 16:42
@kapoisu@pitrou
Update the timestamp of cache correctly
cfce243
@kapoisu@pitrou
Replace std::shared_ptr's constructor with std::make_shared
69ab6b9
@kapoisu@kou@pitrou
Fix typo
463e042
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
@kapoisu@pitrou
Remove the duplicate code from TwoLevelCacheWithExpiration
e23052b
* 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.
@pitrou
pitrou merged commit eda9942 into apache:mainJul 1, 2025
32 of 33 checks passed
@pitroupitrou removed the awaiting committer review Awaiting committer review label Jul 1, 2025
@pitroupitrou mentioned this pull request Jul 1, 2025
Closed
@pitrou

pitrou commented Jul 1, 2025

Copy link
Copy Markdown
Member

@kapoisu Sorry for forgetting about this! I rebased again and then merged. Thank you for your contribution!

@conbench-apache-arrow

conbench-apache-arrowBot commented Jul 2, 2025

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit eda9942.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@kapoisu
kapoisu deleted the GH-46217 branch July 2, 2025 20:40
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adamreeveadamreeveadamreeve approved these changes
@koukoukou approved these changes
@wgtmacwgtmacwgtmac approved these changes

Assignees

No one assigned

Labels

Component: C++Component: Parquet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kapoisu@pitrou@kou@adamreeve@wgtmac