You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kapoisu (via GitHub)" <gi...@apache.org> on 2025/05/01 17:19:37 UTC
[PR] GH-46217: Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctly [arrow]
kapoisu opened a new pull request, #46283:
URL: https://github.com/apache/arrow/pull/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).
### 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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46217: [C++][Parquet] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctly [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #46283:
URL: https://github.com/apache/arrow/pull/46283
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46217: [C++] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctly [arrow]
Posted by "kapoisu (via GitHub)" <gi...@apache.org>.
kapoisu commented on PR #46283:
URL: https://github.com/apache/arrow/pull/46283#issuecomment-2846667075
I'll try to see if I can fix the error during CI. It seems similar to https://github.com/apache/arrow/issues/46203.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46217: [C++][Parquet] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctly [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #46283:
URL: https://github.com/apache/arrow/pull/46283#issuecomment-2945026926
I've rebased on git main and will merge if CI is ok.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46217: [C++][Parquet] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctly [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #46283:
URL: https://github.com/apache/arrow/pull/46283#issuecomment-3024512960
@kapoisu Sorry for forgetting about this! I rebased again and then merged. Thank you for your contribution!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46217: [C++] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctly [arrow]
Posted by "kapoisu (via GitHub)" <gi...@apache.org>.
kapoisu commented on code in PR #46283:
URL: https://github.com/apache/arrow/pull/46283#discussion_r2072853947
##########
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) {
Review Comment:
The parameter indicates that at least cache_cleanup_period_seconds must pass before checking the expired tokens. However, the gap between each call is undetermined thus the paramater doesn't implies the actual cleanup period. The fact that ```CheckCacheForExpiredTokens()``` checks the expired tokens no matter how much time has passed seems reasonable to me. Or maybe rename the parameter to something else?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46217: [C++] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctly [arrow]
Posted by "kapoisu (via GitHub)" <gi...@apache.org>.
kapoisu commented on PR #46283:
URL: https://github.com/apache/arrow/pull/46283#issuecomment-2853366530
The mismatch of schemas during integration tests seems related to https://github.com/apache/arrow/issues/46087. Since the issue was closed a few days ago (via https://github.com/apache/arrow/pull/46110), I rebased my branch to see if the previously failed tests would pass now.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46217: [C++] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctly [arrow]
Posted by "kapoisu (via GitHub)" <gi...@apache.org>.
kapoisu commented on code in PR #46283:
URL: https://github.com/apache/arrow/pull/46283#discussion_r2072853947
##########
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) {
Review Comment:
The parameter indicates that at least cache_cleanup_period_seconds must pass before checking the expired tokens. However, the duration between each call is undetermined thus the paramater doesn't implies the actual cleanup period. ```CheckCacheForExpiredTokens()``` checks the expired tokens no matter how much time has passed seems reasonable to me. Or maybe rename the parameter to something else?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46217: [C++] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctly [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #46283:
URL: https://github.com/apache/arrow/pull/46283#issuecomment-2845298070
:warning: GitHub issue #46217 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46217: [C++][Parquet] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctly [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #46283:
URL: https://github.com/apache/arrow/pull/46283#issuecomment-3026420559
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit eda9942a4d6e78f9017d928ce39346ec2c13d03c.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/45183640727) has more details.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46217: [C++] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctly [arrow]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #46283:
URL: https://github.com/apache/arrow/pull/46283#discussion_r2074660415
##########
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) {
Review Comment:
Sorry yes you're right, I got confused and was thinking the cleanup period was used to determine which entries were expired.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46217: [C++] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctly [arrow]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #46283:
URL: https://github.com/apache/arrow/pull/46283#discussion_r2072720886
##########
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) {
Review Comment:
I think the 0.0 should be passed explicitly in the test rather than being a default value. Having a default cleanup period of 0 seconds doesn't really make sense.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46217: [C++] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctly [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #46283:
URL: https://github.com/apache/arrow/pull/46283#discussion_r2070945897
##########
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
Review Comment:
```suggestion
// CheckCacheForExpiredTokens() should be called at least twice to verify the
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46217: [C++] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctly [arrow]
Posted by "kapoisu (via GitHub)" <gi...@apache.org>.
kapoisu commented on code in PR #46283:
URL: https://github.com/apache/arrow/pull/46283#discussion_r2072853947
##########
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) {
Review Comment:
The parameter indicates that at least cache_cleanup_period_seconds must pass before checking the expired tokens. However, the duration between each call is undetermined thus the paramater doesn't implies the actual cleanup period. ```CheckCacheForExpiredTokens()``` checks the expired tokens no matter how much time has passed seems reasonable to me?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46217: [C++] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctly [arrow]
Posted by "kapoisu (via GitHub)" <gi...@apache.org>.
kapoisu commented on code in PR #46283:
URL: https://github.com/apache/arrow/pull/46283#discussion_r2072853947
##########
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) {
Review Comment:
The parameter indicates that at least cache_cleanup_period_seconds must pass before checking the expired tokens. However, the gap between each call is undetermined thus the paramater doesn't implies the actual cleanup period. ```CheckCacheForExpiredTokens()``` checks the expired tokens no matter how much time has passed seems reasonable to me. Or maybe rename the parameter to something else?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org