fix: Fix ValueError in signal handling for Trino worker threads - #6428
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a147b7de22
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """signal.signal() should work fine in main thread.""" | ||
| cursor = MagicMock() | ||
| # Should not raise any exception in main thread | ||
| query = Query(query_text="SELECT 1", cursor=cursor) |
There was a problem hiding this comment.
Restore process signal handlers after the test
When this test runs, Query.__init__ installs process-global SIGINT/SIGTERM handlers that point at this test's mock cursor, and the test never restores the previous handlers. Any later test or CI shutdown signal in the same pytest process can be intercepted by query.cancel() instead of the runner's normal KeyboardInterrupt/termination handling, which can make the rest of the suite ignore interrupts or hang until forcibly killed. Patch signal.signal in this test or save and restore the old handlers in a finally.
Useful? React with 👍 / 👎.
jyejare
left a comment
There was a problem hiding this comment.
This PR fixes a ValueError that occurs when Query objects are instantiated in worker threads by guarding signal.signal() calls with a main thread check. The fix is simple and effective, with good test coverage demonstrating both main thread and worker thread scenarios.
|
|
||
| assert not errors, f"Unexpected ValueError in worker thread: {errors[0]}" |
There was a problem hiding this comment.
[Suggestion] Improve error message handling
The error message assumes errors[0] exists, but this could cause an IndexError if errors is empty. Consider a more defensive approach.
Current code:
+ assert not errors, f"Unexpected ValueError in worker thread: {errors[0]}"Suggested:
| assert not errors, f"Unexpected ValueError in worker thread: {errors[0]}" | |
| assert not errors, f"Unexpected ValueError in worker thread: {errors[0] if errors else 'No errors captured'}" |
There was a problem hiding this comment.
Thanks for pointing it out. To avoid index lookups entirely and make the error reporting more robust (in case multiple worker threads fail at once), I've updated it to print the entire errors list instead. Let me know what you think!
Good catch. Made the change |
|
Go Vulnerability Check failures are pre-existing CVEs in go-feature-server dependencies, unrelated to this Python-only change. Happy to rebase if needed https://github.com/feast-dev/feast/actions/runs/26874419277/job/79258191188?pr=6428 |
|
The unit test failure (test_cli_date_combinations) seems like a CI environment issue — psutil version mismatch in the macos-14 runner's cached venv, unrelated to this change. Could a maintainer re-run the job or clear the cache? https://github.com/feast-dev/feast/actions/runs/26874419345/job/79258190549?pr=6428 |
18100c9 to
a1bc586
Compare
|
Can we have another approval ? |
please sign the commit to fix DCO check |
a1bc586 to
4a945f2
Compare
This is done, could i request another approval? |
Signed-off-by: MBM <mihir105@gmail.com>
Signed-off-by: MBM <mihir105@gmail.com>
…ore/tests/test_trino_queries.py Apply reviewer suggestion: verify correct signals and handler in test Co-authored-by: Jitendra Yejare <jyejare@redhat.com> Signed-off-by: MBM <mihir105@gmail.com>
- Simplify worker thread assertion message to safely print errors list - Add verification for signal handler registration details Signed-off-by: MBM <mihir105@gmail.com>
4a945f2 to
4c1e5d8
Compare
# [0.65.0](v0.64.0...v0.65.0) (2026-07-20) ### Bug Fixes * add debug logging for FIPS mode detection fallback ([6c1b24e](6c1b24e)) * Build embedded UI from local source ([#6525](#6525)) ([3500349](3500349)) * Bump decommissioned Snowflake Python UDF runtime from 3.9 to 3.10 ([#6606](#6606)) ([#6608](#6608)) ([10341e4](10341e4)) * configure FIPS-compliant gRPC cipher suites for offline server ([6bc80a2](6bc80a2)) * Correct Flink PyArrow dependency constraints ([#6604](#6604)) ([70a9751](70a9751)) * Fix ValueError in signal handling for Trino worker threads ([#6428](#6428)) ([506d919](506d919)) * Fixed monitoring page issues ([7946018](7946018)) * Make pytest config compatible with newer pytest ([#5779](#5779)) ([a57ea33](a57ea33)) * Replace comma with space in DynamoDB-incompatible label tag value ([51e3a16](51e3a16)) * Resolve UI build warnings ([#6529](#6529)) ([abe92af](abe92af)) * Unblock nightly UI build ([#6570](#6570)) ([f296d4b](f296d4b)) * Use LONGBLOB for SQL registry proto columns on MySQL ([#6566](#6566)) ([7e4beb2](7e4beb2)) ### Features * Add click-to-zoom lightbox for blog post images ([#6575](#6575)) ([1cb23fd](1cb23fd)) * Add dark mode support to website and blog ([#6589](#6589)) ([7358fb8](7358fb8)) * Add OnlineStore for Aerospike ([#6532](#6532)) ([9cd35e1](9cd35e1)) * Add OpenLineage Consumer to Feast - receive, store, and visualize cross-producer lineage ([#6549](#6549)) ([a834126](a834126)) * Add registry list feature views by updated since ([#6092](#6092)) ([#6093](#6093)) ([006c606](006c606)) * Add ScyllaDB online store with vector search ([#6508](#6508)) ([1669661](1669661)) * Added compute and jobs UI ([ba2c05c](ba2c05c)) * Added Iceberg REST Catalog data source support ([e0a8573](e0a8573)) * Bring Your Own Spark - SparkApplication ([#6550](#6550)) ([dcd496f](dcd496f)) * **cassandra:** Add multi-DC support via per-datacenter execution profiles ([#6434](#6434)) ([0de9196](0de9196)) * Enhanced data source creation as a visual catalog with type-specific forms ([#6557](#6557)) ([d6acbba](d6acbba)) * Enhanced datasets UI functionality ([de11152](de11152)) * Implement RegistryServer.Proto RPC with RBAC-filtered response ([#6558](#6558)) ([#6552](#6552)) ([0d02614](0d02614)) * New zoned timestamp feature type ([#6536](#6536)) ([#6537](#6537)) ([eb042f0](eb042f0)) * **operator:** Auto-create RBAC for spark_application batch engine ([#6597](#6597)) ([f487b37](f487b37)) * **operator:** integrate cluster TLS profile for OCP 5.0 compliance ([43263a6](43263a6)) * Permissions CRUD UI and OIDC auth integration in UI ([6511da1](6511da1)) * Retrieve historical features from BigQuery without entity_df ([#6569](#6569)) ([cd5f6bb](cd5f6bb)), closes [#6558](#6558) [#6552](#6552) * **spark:** SparkSource query+path and pre-computed offline read for BatchFeatureView ([#6440](#6440)) ([4dc8757](4dc8757)) ### BREAKING CHANGES * total_timeout_ms is renamed to batch_total_timeout_ms. Config files using the old name must be updated. No default value change. Docs updated (reference + perf-tuning guide) with a short explainer on the per-attempt vs total deadline distinction. Two new unit tests pin the policy wiring: socket_timeout_ms propagates to all three scopes, and is omitted (not injected as None) when unset. Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * refactor(aerospike): use MAP_KEY_ORDERED, KEY_DIGEST, and instance-scoped client Cheap-win cleanups flagged in review, all touching the same small patch of write-path and lifecycle code. * Map CDTs are now created with MAP_KEY_ORDERED. map_get_by_key / map_remove_by_key on an ordered map are O(log N) in the map size instead of O(N); matters on reads of wide feature views and on the update() background scan (which walks every record in the project's set). * Writes drop POLICY_KEY_SEND and rely on the client default (POLICY_KEY_DIGEST). The serialized entity key is no longer stored alongside each record, saving per-record storage the read path never consumes (batch_operate preserves request order; results are paired back by zip in online_read). * _client moves from a class attribute to an instance attribute (set in __init__). Previously two AerospikeOnlineStore instances could share the cached client through class state until one wrote self._client. With the instance attribute the state is always per-instance from construction. * Drop MongoDB references from class docstrings and comments (they referred to how the storage layout was derived rather than documenting current behavior). Also rewrite the _build_batch_writes docstring to describe the policies applied on the write path. Unit test assertions for the write-path record are updated: bw.policy is now None (client default applies) and map ops carry map_policy={'map_order': MAP_KEY_ORDERED}. All three docker-backed integration tests still pass end-to-end (cross-FV upsert, update() background scan, full feature-store round-trip), so the read/write shape survives the ordering and policy changes against a real server. Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * feat(aerospike): add per-FV namespace/set overrides and prewriting hook Adds three configuration knobs to AerospikeOnlineStoreConfig: - namespace_overrides: pin individual feature views to a different Aerospike namespace (e.g. RAM-only vs. SSD-backed) without splitting the project across stores. - set_overrides: place a feature view in its own set so admin ops on it (truncate, scan-based deletes during `feast apply`) do not touch records of other views. - prewriting_hook: import-string-resolved callable invoked once per online_write_batch with the rows about to be written, returning the rows that actually go on the wire. Resolved and cached on first use; returning [] short-circuits the wire call. Read, write, update and teardown paths all honour the per-FV ns/set resolution. update() groups dropped feature views by their resolved (ns, set) pair and issues one background scan per group. teardown() truncates every unique (ns, set) pair the project may have written to, including the store-level default. Adds 22 unit tests for the new behaviour and updates 3 existing call sites of _build_batch_writes for the new namespace= parameter. Adds a sample hook module under examples/online_store/aerospike_overrides_and_hooks/ and corresponding sections in docs/reference/online-stores/aerospike.md. Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * test: update aerospike image tag Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * chore: sync README template and secrets baseline after master merge Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * chore: fix secrets baseline line number for v1 operator types Adding aerospike to the feast-operator enum shifted the allowlisted SecretRef entry in api/v1/featurestore_types.go by one line. Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * docs: update aerospike docs Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * fix(aerospike): wire batch max_retries and fix empty projection handling Copilot review feedback on PR #6532: - Add max_retries to the batch client policy (batch_operate/batch_write path) - Treat empty projected feature maps as present FV slots (is not None) - Return {} from _normalize_projected_features([]) instead of None - Fix projection unit test mock/assertions - Correct prewriting_hook config docstring Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * style(aerospike): format online_read docs assignment for ruff Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * chore: update pixi.lock for aerospike optional extra Regenerate the v6 lockfile with Pixi v0.63.1 after adding the aerospike extra to pyproject.toml. Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * fix(aerospike): add client init lock and batch chunking Guard lazy client creation with a lock to avoid connection leaks under concurrent first use, and chunk batch reads/writes by batch_max_records so large materializations stay under Aerospike server batch limits. Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com>
# [0.65.0](feast-dev/feast@v0.64.0...v0.65.0) (2026-07-20) ### Bug Fixes * add debug logging for FIPS mode detection fallback ([6c1b24e](feast-dev@6c1b24e)) * Build embedded UI from local source ([feast-dev#6525](feast-dev#6525)) ([3500349](feast-dev@3500349)) * Bump decommissioned Snowflake Python UDF runtime from 3.9 to 3.10 ([feast-dev#6606](feast-dev#6606)) ([feast-dev#6608](feast-dev#6608)) ([10341e4](feast-dev@10341e4)) * configure FIPS-compliant gRPC cipher suites for offline server ([6bc80a2](feast-dev@6bc80a2)) * Correct Flink PyArrow dependency constraints ([feast-dev#6604](feast-dev#6604)) ([70a9751](feast-dev@70a9751)) * Fix ValueError in signal handling for Trino worker threads ([feast-dev#6428](feast-dev#6428)) ([506d919](feast-dev@506d919)) * Fixed monitoring page issues ([7946018](feast-dev@7946018)) * Make pytest config compatible with newer pytest ([feast-dev#5779](feast-dev#5779)) ([a57ea33](feast-dev@a57ea33)) * Replace comma with space in DynamoDB-incompatible label tag value ([51e3a16](feast-dev@51e3a16)) * Resolve UI build warnings ([feast-dev#6529](feast-dev#6529)) ([abe92af](feast-dev@abe92af)) * Unblock nightly UI build ([feast-dev#6570](feast-dev#6570)) ([f296d4b](feast-dev@f296d4b)) * Use LONGBLOB for SQL registry proto columns on MySQL ([feast-dev#6566](feast-dev#6566)) ([7e4beb2](feast-dev@7e4beb2)) ### Features * Add click-to-zoom lightbox for blog post images ([feast-dev#6575](feast-dev#6575)) ([1cb23fd](feast-dev@1cb23fd)) * Add dark mode support to website and blog ([feast-dev#6589](feast-dev#6589)) ([7358fb8](feast-dev@7358fb8)) * Add OnlineStore for Aerospike ([feast-dev#6532](feast-dev#6532)) ([9cd35e1](feast-dev@9cd35e1)) * Add OpenLineage Consumer to Feast - receive, store, and visualize cross-producer lineage ([feast-dev#6549](feast-dev#6549)) ([a834126](feast-dev@a834126)) * Add registry list feature views by updated since ([feast-dev#6092](feast-dev#6092)) ([feast-dev#6093](feast-dev#6093)) ([006c606](feast-dev@006c606)) * Add ScyllaDB online store with vector search ([feast-dev#6508](feast-dev#6508)) ([1669661](feast-dev@1669661)) * Added compute and jobs UI ([ba2c05c](feast-dev@ba2c05c)) * Added Iceberg REST Catalog data source support ([e0a8573](feast-dev@e0a8573)) * Bring Your Own Spark - SparkApplication ([feast-dev#6550](feast-dev#6550)) ([dcd496f](feast-dev@dcd496f)) * **cassandra:** Add multi-DC support via per-datacenter execution profiles ([feast-dev#6434](feast-dev#6434)) ([0de9196](feast-dev@0de9196)) * Enhanced data source creation as a visual catalog with type-specific forms ([feast-dev#6557](feast-dev#6557)) ([d6acbba](feast-dev@d6acbba)) * Enhanced datasets UI functionality ([de11152](feast-dev@de11152)) * Implement RegistryServer.Proto RPC with RBAC-filtered response ([feast-dev#6558](feast-dev#6558)) ([feast-dev#6552](feast-dev#6552)) ([0d02614](feast-dev@0d02614)) * New zoned timestamp feature type ([feast-dev#6536](feast-dev#6536)) ([feast-dev#6537](feast-dev#6537)) ([eb042f0](feast-dev@eb042f0)) * **operator:** Auto-create RBAC for spark_application batch engine ([feast-dev#6597](feast-dev#6597)) ([f487b37](feast-dev@f487b37)) * **operator:** integrate cluster TLS profile for OCP 5.0 compliance ([43263a6](feast-dev@43263a6)) * Permissions CRUD UI and OIDC auth integration in UI ([6511da1](feast-dev@6511da1)) * Retrieve historical features from BigQuery without entity_df ([feast-dev#6569](feast-dev#6569)) ([cd5f6bb](feast-dev@cd5f6bb)), closes [feast-dev#6558](feast-dev#6558) [feast-dev#6552](feast-dev#6552) * **spark:** SparkSource query+path and pre-computed offline read for BatchFeatureView ([feast-dev#6440](feast-dev#6440)) ([4dc8757](feast-dev@4dc8757)) ### BREAKING CHANGES * total_timeout_ms is renamed to batch_total_timeout_ms. Config files using the old name must be updated. No default value change. Docs updated (reference + perf-tuning guide) with a short explainer on the per-attempt vs total deadline distinction. Two new unit tests pin the policy wiring: socket_timeout_ms propagates to all three scopes, and is omitted (not injected as None) when unset. Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * refactor(aerospike): use MAP_KEY_ORDERED, KEY_DIGEST, and instance-scoped client Cheap-win cleanups flagged in review, all touching the same small patch of write-path and lifecycle code. * Map CDTs are now created with MAP_KEY_ORDERED. map_get_by_key / map_remove_by_key on an ordered map are O(log N) in the map size instead of O(N); matters on reads of wide feature views and on the update() background scan (which walks every record in the project's set). * Writes drop POLICY_KEY_SEND and rely on the client default (POLICY_KEY_DIGEST). The serialized entity key is no longer stored alongside each record, saving per-record storage the read path never consumes (batch_operate preserves request order; results are paired back by zip in online_read). * _client moves from a class attribute to an instance attribute (set in __init__). Previously two AerospikeOnlineStore instances could share the cached client through class state until one wrote self._client. With the instance attribute the state is always per-instance from construction. * Drop MongoDB references from class docstrings and comments (they referred to how the storage layout was derived rather than documenting current behavior). Also rewrite the _build_batch_writes docstring to describe the policies applied on the write path. Unit test assertions for the write-path record are updated: bw.policy is now None (client default applies) and map ops carry map_policy={'map_order': MAP_KEY_ORDERED}. All three docker-backed integration tests still pass end-to-end (cross-FV upsert, update() background scan, full feature-store round-trip), so the read/write shape survives the ordering and policy changes against a real server. Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * feat(aerospike): add per-FV namespace/set overrides and prewriting hook Adds three configuration knobs to AerospikeOnlineStoreConfig: - namespace_overrides: pin individual feature views to a different Aerospike namespace (e.g. RAM-only vs. SSD-backed) without splitting the project across stores. - set_overrides: place a feature view in its own set so admin ops on it (truncate, scan-based deletes during `feast apply`) do not touch records of other views. - prewriting_hook: import-string-resolved callable invoked once per online_write_batch with the rows about to be written, returning the rows that actually go on the wire. Resolved and cached on first use; returning [] short-circuits the wire call. Read, write, update and teardown paths all honour the per-FV ns/set resolution. update() groups dropped feature views by their resolved (ns, set) pair and issues one background scan per group. teardown() truncates every unique (ns, set) pair the project may have written to, including the store-level default. Adds 22 unit tests for the new behaviour and updates 3 existing call sites of _build_batch_writes for the new namespace= parameter. Adds a sample hook module under examples/online_store/aerospike_overrides_and_hooks/ and corresponding sections in docs/reference/online-stores/aerospike.md. Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * test: update aerospike image tag Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * chore: sync README template and secrets baseline after master merge Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * chore: fix secrets baseline line number for v1 operator types Adding aerospike to the feast-operator enum shifted the allowlisted SecretRef entry in api/v1/featurestore_types.go by one line. Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * docs: update aerospike docs Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * fix(aerospike): wire batch max_retries and fix empty projection handling Copilot review feedback on PR feast-dev#6532: - Add max_retries to the batch client policy (batch_operate/batch_write path) - Treat empty projected feature maps as present FV slots (is not None) - Return {} from _normalize_projected_features([]) instead of None - Fix projection unit test mock/assertions - Correct prewriting_hook config docstring Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * style(aerospike): format online_read docs assignment for ruff Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * chore: update pixi.lock for aerospike optional extra Regenerate the v6 lockfile with Pixi v0.63.1 after adding the aerospike extra to pyproject.toml. Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com> * fix(aerospike): add client init lock and batch chunking Guard lazy client creation with a lock to avoid connection leaks under concurrent first use, and chunk batch reads/writes by batch_max_records so large materializations stay under Aerospike server batch limits. Signed-off-by: Valentyn Kahamlyk <valentin.kagamlyk@gmail.com>
What this PR does / why we need it:
Guards
signal.signal()calls inQuery.__init__()with athreading.current_thread() is threading.main_thread()check.When the Trino offline store is served via Arrow Flight,
Query.__init__()runs in a worker thread. Python restrictssignal.signal()to the main thread only, causing:ValueError: signal only works in main thread of the main interpreterWhich issue(s) this PR fixes:
Fixes #6424
Checks
git commit -s)Testing Strategy
Misc
Added regression tests covering both main thread and worker thread scenarios.
Signal handling is preserved for CLI/interactive usage. Worker thread usage silently skips signal registration, which is the correct behavior when running under Arrow Flight.