Skip to content

fix: refresh ttlcache upon failures #964

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Apr 11, 2025
Merged

fix: refresh ttlcache upon failures #964

merged 3 commits into from
Apr 11, 2025

Conversation

hzding621
Copy link
Collaborator

@hzding621 hzding621 commented Apr 9, 2025 •
edited
Loading

Summary

Call TTLCache.refresh for all scenarios where returned result is a Failure to avoid it being cached. Updated all places where TTLCache.apply is called.

Why / Goal

To avoid a Failure to be cached for 2 hours. In previous PR #955, we've adjusted the getString behavior so that instead of throw it returns a Failure object.

Test Plan

  • Added Unit Tests
  • Covered by existing CI
  • Integration tested

Checklist

  • Documentation update

Reviewers

@airbnb/airbnb-chronon-maintainers

@hzding621 hzding621 force-pushed the haozhen--ttl-refresh branch 4 times, most recently from e2e82f7 to b02ae8e Compare April 9, 2025 23:35
@hzding621 hzding621 closed this Apr 10, 2025
Haozhen Ding added 3 commits April 10, 2025 12:10
Reapply "fix: propagate kvstore multi get exception properly (#955)" (#…
6fdc8e6 
…965)

This reverts commit 5765eb6.
add UT
260d58a 
fix FlexibleExecutionContext

refresh ttl when failure
use vanilla try-catch
8588f06
@hzding621 hzding621 reopened this Apr 10, 2025
@hzding621 hzding621 force-pushed the haozhen--ttl-refresh branch from b02ae8e to 8588f06 Compare April 10, 2025 23:30
@hzding621 hzding621 merged commit 302b496 into main Apr 11, 2025
7 checks passed
@hzding621 hzding621 deleted the haozhen--ttl-refresh branch April 11, 2025 06:03
@piyush-zlai piyush-zlai mentioned this pull request Apr 30, 2025
Merged
4 tasks
piyush-zlai added a commit to zipline-ai/chronon that referenced this pull request Apr 30, 2025
@piyush-zlai
Cherrypick OSS fetcher failure handling PRs - #932 and #964 (#706)
8f6ee6c 
## Summary
Pull in PRs - airbnb/chronon#964 and
airbnb/chronon#932. We hit issues related to 964
in some of our tests at Etsy - groupByServingInfo lookups against BT
timed out and we end up caching the failure response. 964 addresses this
and it depends on 932 so pulling that in as well.

## Checklist
- [ ] Added Unit Tests
- [X] Covered by existing CI
- [ ] Integration tested
- [ ] Documentation update



<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Improved error handling and reporting for partial failures in join
operations and key-value store lookups.
- Enhanced cache refresh mechanisms for join configurations and
metadata, improving system robustness during failures.
- Added a configurable option to control strictness on invalid dataset
references in the in-memory key-value store.

- **Bug Fixes**
- Exceptions and partial failures are now more accurately surfaced in
fetch responses, ensuring clearer diagnostics for end-users.
	- Updated error key naming for consistency in response maps.

- **Tests**
- Added a new test to verify correct handling and reporting of partial
failures in key-value store operations.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
kumar-zlai pushed a commit to zipline-ai/chronon that referenced this pull request May 1, 2025
@piyush-zlai
Cherrypick OSS fetcher failure handling PRs - #932 and #964 (#706)
db26796 
## Summary
Pull in PRs - airbnb/chronon#964 and
airbnb/chronon#932. We hit issues related to 964
in some of our tests at Etsy - groupByServingInfo lookups against BT
timed out and we end up caching the failure response. 964 addresses this
and it depends on 932 so pulling that in as well.

## Checklist
- [ ] Added Unit Tests
- [X] Covered by existing CI
- [ ] Integration tested
- [ ] Documentation update



<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Improved error handling and reporting for partial failures in join
operations and key-value store lookups.
- Enhanced cache refresh mechanisms for join configurations and
metadata, improving system robustness during failures.
- Added a configurable option to control strictness on invalid dataset
references in the in-memory key-value store.

- **Bug Fixes**
- Exceptions and partial failures are now more accurately surfaced in
fetch responses, ensuring clearer diagnostics for end-users.
	- Updated error key naming for consistency in response maps.

- **Tests**
- Added a new test to verify correct handling and reporting of partial
failures in key-value store operations.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
chewy-zlai pushed a commit to zipline-ai/chronon that referenced this pull request May 15, 2025
@piyush-zlai
Cherrypick OSS fetcher failure handling PRs - #932 and #964 (#706)
1b469c9 
## Summary
Pull in PRs - airbnb/chronon#964 and
airbnb/chronon#932. We hit issues related to 964
in some of our tests at our clients - groupByServingInfo lookups against BT
timed out and we end up caching the failure response. 964 addresses this
and it depends on 932 so pulling that in as well.

## Checklist
- [ ] Added Unit Tests
- [X] Covered by existing CI
- [ ] Integration tested
- [ ] Documentation update



<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Improved error handling and reporting for partial failures in join
operations and key-value store lookups.
- Enhanced cache refresh mechanisms for join configurations and
metadata, improving system robustness during failures.
- Added a configurable option to control strictness on invalid dataset
references in the in-memory key-value store.

- **Bug Fixes**
- Exceptions and partial failures are now more accurately surfaced in
fetch responses, ensuring clearer diagnostics for end-users.
	- Updated error key naming for consistency in response maps.

- **Tests**
- Added a new test to verify correct handling and reporting of partial
failures in key-value store operations.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
chewy-zlai pushed a commit to zipline-ai/chronon that referenced this pull request May 15, 2025
@piyush-zlai
Cherrypick OSS fetcher failure handling PRs - #932 and #964 (#706)
5bdbba4 
## Summary
Pull in PRs - airbnb/chronon#964 and
airbnb/chronon#932. We hit issues related to 964
in some of our tests at our clients - groupByServingInfo lookups against BT
timed out and we end up caching the failure response. 964 addresses this
and it depends on 932 so pulling that in as well.

## Checklist
- [ ] Added Unit Tests
- [X] Covered by existing CI
- [ ] Integration tested
- [ ] Documentation update



<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Improved error handling and reporting for partial failures in join
operations and key-value store lookups.
- Enhanced cache refresh mechanisms for join configurations and
metadata, improving system robustness during failures.
- Added a configurable option to control strictness on invalid dataset
references in the in-memory key-value store.

- **Bug Fixes**
- Exceptions and partial failures are now more accurately surfaced in
fetch responses, ensuring clearer diagnostics for end-users.
	- Updated error key naming for consistency in response maps.

- **Tests**
- Added a new test to verify correct handling and reporting of partial
failures in key-value store operations.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
chewy-zlai pushed a commit to zipline-ai/chronon that referenced this pull request May 16, 2025
@piyush-zlai
Cherrypiour clients OSS fetcher failure handling PRs - #932 and #964 (#…
90e0d27 
…706)

## Summary
Pull in PRs - airbnb/chronon#964 and
airbnb/chronon#932. We hit issues related to 964
in some of our tests at our clients - groupByServingInfo lookups against BT
timed out and we end up caching the failure response. 964 addresses this
and it depends on 932 so pulling that in as well.

## Cheour clientslist
- [ ] Added Unit Tests
- [X] Covered by existing CI
- [ ] Integration tested
- [ ] Documentation update



<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Improved error handling and reporting for partial failures in join
operations and key-value store lookups.
- Enhanced cache refresh mechanisms for join configurations and
metadata, improving system robustness during failures.
- Added a configurable option to control strictness on invalid dataset
references in the in-memory key-value store.

- **Bug Fixes**
- Exceptions and partial failures are now more accurately surfaced in
fetch responses, ensuring clearer diagnostics for end-users.
	- Updated error key naming for consistency in response maps.

- **Tests**
- Added a new test to verify correct handling and reporting of partial
failures in key-value store operations.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pkundurthy pkundurthy pkundurthy approved these changes

@pengyu-hou pengyu-hou pengyu-hou approved these changes

@yuli-han yuli-han yuli-han approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hzding621 @pkundurthy @pengyu-hou @yuli-han