bito-code-review[bot] commented on code in PR #44714:
URL: https://github.com/apache/superset/pull/44714#discussion_r4130345407
##########
superset/db_engine_specs/impala.py:
##########
@@ -128,6 +128,27 @@ def execute(
except Exception as ex:
raise cls.get_dbapi_mapped_exception(ex) from ex
+ @classmethod
+ def fetch_data(cls, cursor: Any, limit: int | None = None) ->
list[tuple[Any, ...]]:
+ """Wait for asynchronous operations using the public cursor API."""
+ if callable(getattr(cursor, "execute_async", None)):
+ from impala.error import Error
+
+ deadline = time.monotonic() + app.config["SQLLAB_TIMEOUT"]
+ try:
+ while cursor.is_executing():
+ if time.monotonic() >= deadline:
+ cursor.cancel_operation()
+ raise TimeoutError("Timed out waiting for the Impala
operation")
+ time.sleep(0.1)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Blocking 0.1s poll loop</b></div>
<div id="fix">
This busy-wait blocks the SQL Lab worker up to `SQLLAB_TIMEOUT` (30s
default) with fixed 0.1s sleeps, and it duplicates polling that `handle_cursor`
already performs via `cursor.status()` with `DB_POLL_INTERVAL_SECONDS`.
impyla's own `_wait_to_finish` uses an adaptive sleep interval (0.01s-1s)
around the same `GetOperationStatus` RPC. Consider reusing that cadence instead
of a hardcoded 0.1s.
</div>
</div>
<div id="suggestion">
<div id="issue"><b>Cancel no-op on timeout</b></div>
<div id="fix">
`cancel_operation()` is a no-op when impyla's `_last_operation_active` is
False (impyla 0.24.0 `hiveserver2.py:322-328`), which happens if the operation
leaves the executing state between the `is_executing()` check and this call, or
after a prior `_reset_state()`. The driver `Error` from a real cancel attempt
would also be swallowed by the outer `except Error` and misreported as a
timeout. Guard the cancel and keep raising `TimeoutError`.
</div>
</div>
<small><i>Code Review Run #4d9f64</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
