-
Notifications
You must be signed in to change notification settings - Fork 30
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 BulkResponse
wrapper for improved decoding of HTTP bulk responses
#649
base: master
Are you sure you want to change the base?
Conversation
src/crate/client/tests.py
Outdated
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.
tests.py
is the entrypoint for zope.testing to discover test cases. Many utility functions had to be refactored away from here, in order to avoid circular imports.
7ef36cd
to
ccbffd2
Compare
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.
Thx for improving the bulk response handling. Left some comments.
ccbffd2
to
ea27b3e
Compare
50347d4
to
433dfdf
Compare
CrateDB HTTP bulk responses include `rowcount=` items, either signalling if a bulk operation succeeded or failed. - success means `rowcount=1` - failure means `rowcount=-2` https://cratedb.com/docs/crate/reference/en/latest/interfaces/http.html#error-handling
433dfdf
to
4da893a
Compare
4da893a
to
6a58353
Compare
self.assertEqual(result, [{"rowcount": 1}, {"rowcount": -2}]) | ||
|
||
# Verify decoded response. | ||
bulk_response = BulkResponse(invalid_records, result) |
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.
Looking at this more carefully, I don't like that BulkResponse is something you need con construct manually. Couldn't we directly return it from the insert execution, instead of a list of BulkResultItem
?
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.
Hi. I don't think we can do anything like this here, because the Python database driver must adhere to the Python Database API Specification, so the BulkResponse
is just meant as an optional extension to it.
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.
af32409, just added, provides a bit of documentation for that extension in the section about bulk operations.
Newer machines don't have `python`, just `python2` and `python3`.
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.
Could you elaborate a bit more on the motivation for this?
Seems to me as if getting the info without this is not really that much more difficult.
Hi @mfussenegger,
The idea is to have a concise code representation over here, for the Because CrateDB Toolkit is rather heavy, and I wanted to make it reusable, I wanted to add it elsewhere. Another option would be to use the now separate cratedb-sqlalchemy package. |
That kinda confirms my point in that there's not much more code without the BulkResponse wrapper: cursor = self.connection.execute(statement=statement, parameters=operation.parameters)
self.connection.commit()
cratedb_bulk_result = getattr(cursor.context, "last_executemany_result", None)
failed_records = 0
success_count = 0
for result in cratedb_bulk_result:
if result["rowcount"] == -2:
failed_records += 1
else:
success_count += 1
self._metrics.count_success_total += success_count
self.progress_bar and self.progress_bar.update(n=success_count) This to me would even be more readable as it avoids the additional indirection. Also, given that the DB API spec says:
I think we could extend our return value - as long as we can keep it compatible with the existing one. |
About
CrateDB HTTP bulk responses include
rowcount=
items, either signalling if a bulk operation succeeded or failed.rowcount=1
rowcount=-2
https://cratedb.com/docs/crate/reference/en/latest/interfaces/http.html#error-handling
References
The code is coming from CrateDB Toolkit, but is generally usable beyond there.