-
Notifications
You must be signed in to change notification settings - Fork 84
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
[PECO-1263] Separate get_status and poll_for_status methods #313
Conversation
Signed-off-by: Jesse Whitehouse <jesse.whitehouse@databricks.com>
src/databricks/sql/ae.py
Outdated
"""Check the thrift server for the status of this operation and set self.status | ||
|
||
This will result in an error if the operation has been canceled or aborted at the server""" | ||
self._thrift_get_operation_status() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just an alias. Should I just rename the _thrift_get_operation_status
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My instinct is yes, just rename the other method. The more nuanced answer is, it depends on if you think the private version of the method will evolve in ways that the public version could be refactored to adapt for without changing the interface. The benefit of a private method is that you should feel comfortable changing private interfaces without breaking consumers...however python doesn't really enforce private, so the waters get muddied. So yeah, I say just rename the existing method.
arrow_table, | ||
num_rows, | ||
) = convert_arrow_based_set_to_arrow_table( | ||
(arrow_table, num_rows,) = convert_arrow_based_set_to_arrow_table( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a black
reformat unrelated to this PR.
class TestExecuteAsync(PySQLPytestTestCase): | ||
@pytest.fixture | ||
def long_running_ae(self, scope="function") -> AsyncExecution: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For simplicity I added this pytest fixture so I don't need to duplicate this code for my "raises an exception" assertions.
Signed-off-by: Jesse Whitehouse <jesse.whitehouse@databricks.com>
Description
Per guidance from @benc-db, I've separated the
get_result_or_status
method into separate calls. The hello world for this interface now looks like this:What's next?
I need to teach AsyncExecution how to pick up a running execution that was started by another thread.