Skip to content

fix: Add possibility to overwrite send_receive_timeout for clickhouse offline store #5792

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
ntkathole merged 4 commits into from
Dec 23, 2025
Merged

fix: Add possibility to overwrite send_receive_timeout for clickhouse offline store #5792

ntkathole merged 4 commits into from
Dec 23, 2025

Conversation

@astronautas
Copy link
Contributor

@astronautas astronautas commented Dec 22, 2025

What this PR does / why we need it:

We noticed that for some of our larger get_historical_features jobs, we started getting:

[2025-12-19, 14:16:36 UTC] {pod_manager.py:454} INFO - [base] OperationalError: Error
[2025-12-19, 14:16:36 UTC] {pod_manager.py:454} INFO - [base] HTTPConnectionPool(host='cluster-clickhouse-ml-main.ml.svc.cluster.local',
[2025-12-19, 14:16:36 UTC] {pod_manager.py:454} INFO - [base] port=8123): Read timed out. (read timeout=300) executing HTTP request attempt 1

in essence, we need to be able to control some of the client-side timeouts within the clickhouse client used by the feature store. PR introduces such functionality.

Which issue(s) this PR fixes:

(inline issue within this PR)

@astronautas astronautas requested a review from a team as a code owner December 22, 2025 10:32
@astronautas
Copy link
Contributor Author

astronautas commented Dec 22, 2025 •
edited
Loading

@franciscojavierarceo @ntkathole A small improvement to Clickhouse offline store.

@astronautas astronautas force-pushed the fix/control-clickhouse-offline-store-client-timeouts branch 2 times, most recently from a9c9902 to 508bf0f Compare December 22, 2025 10:44
ntkathole
ntkathole reviewed Dec 22, 2025
View reviewed changes
@astronautas
Copy link
Contributor Author

astronautas commented Dec 22, 2025 •
edited
Loading

I'll additionally test how this more generic additional_args field works with feature_store.yaml config 🕐 . Will let you know in this thread @ntkathole .

@astronautas
Copy link
Contributor Author

astronautas commented Dec 22, 2025 •
edited
Loading

@ntkathole All ready from my end!

@astronautas
Copy link
Contributor Author

astronautas commented Dec 22, 2025 •
edited
Loading

@ntkathole Any ideas?

Run make test-python-integration
python -m pytest --tb=short -v -n 8 --integration --color=yes --durations=10 --timeout=1200 --timeout_method=thread --dist loadgroup \
	-k "(not snowflake or not test_historical_features_main)" \
	-m "not rbac_remote_integration_test" \
	--log-cli-level=INFO -s \
	sdk/python/tests
/opt/hostedtoolcache/Python/3.11.14/x64/bin/python: No module named pytest
make: *** [Makefile:157: test-python-integration] Error 1
Error: Process completed with exit code 2.

can we retry? Unrelated to PR, it seems :/ Caching?

@ntkathole
Copy link
Member

ntkathole commented Dec 22, 2025

It seems related to aws creds, looking
Error: The security token included in the request is invalid.

@astronautas @ntkathole
add changes + fix linting issues
2f5a1c1 
Signed-off-by: lukas.valatka <lukas.valatka@cast.ai>
@astronautas @ntkathole
generalize based on pr suggestion
24f6f40 
Signed-off-by: lukas.valatka <lukas.valatka@cast.ai>
@astronautas @ntkathole
add test verifying config parsing
a05c6ab 
Signed-off-by: lukas.valatka <lukas.valatka@cast.ai>
@astronautas @ntkathole
rework test into integration, since CH instance is needed
a3b9977 
Signed-off-by: lukas.valatka <lukas.valatka@cast.ai>
@ntkathole ntkathole force-pushed the fix/control-clickhouse-offline-store-client-timeouts branch from 15c6d7a to a3b9977 Compare December 23, 2025 07:26
ntkathole
ntkathole approved these changes Dec 23, 2025
View reviewed changes
Copy link
Member

@ntkathole ntkathole left a comment

Choose a reason for hiding this comment

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

looks good

@ntkathole ntkathole merged commit 59dbb33 into feast-dev:master Dec 23, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ntkathole ntkathole ntkathole approved these changes

Assignees

No one assigned

Labels

ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@astronautas @ntkathole