Skip to content
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.

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

Add explicit heartbeat interval for GetOperationStatus #385

Closed
wants to merge 1 commit into from

Conversation

benc-db
Copy link
Collaborator

@benc-db benc-db commented Apr 5, 2024

In my investigation, it looks as though the current interval between GetOperationStatus requests is enforced by the server, as it comes between sending and receiving the request, and turning on debug logging for urllib3 shows no retries. This PR adds an entry point for configuring how long to wait client side between sending successive GetOperationStatus requests on 200 response. I'm initially proposing to set this value at 25 seconds, so that when adding to the 5s server-side, we are polling every 30s for statement completion.

Copy link

github-actions bot commented Apr 5, 2024

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

Signed-off-by: Ben Cassell <ben.cassell@databricks.com>
@@ -55,6 +55,7 @@

TIMESTAMP_AS_STRING_CONFIG = "spark.thriftserver.arrowBasedRowSet.timestampAsString"
DEFAULT_SOCKET_TIMEOUT = float(900)
DEFAULT_STATEMENT_HEARTBEAT_INTERVAL = float(25)
Copy link
Collaborator

@kravets-levko kravets-levko Apr 5, 2024

Choose a reason for hiding this comment

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

For me 25 seconds delay between GetOperationStatus requests looks a bit too much. I mean - if query execution finishes faster than this delay - client will still wait that 25 seconds. In Nodejs we poll for operation status with 100ms interval - which may be too low, but looks way more reasonable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

100ms? Do you actually see that in practice? Today we wait 0, and the server holds up for up to 5 seconds. Seems weird to care about subsecond latency on queries that we know take more than 5 seconds.

@benc-db benc-db marked this pull request as draft April 5, 2024 21:31
@benc-db benc-db closed this Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants