Make BulkIndexError and ScanError serializable #2669
Conversation
|
💚 CLA has been signed |
|
A documentation preview will be available soon. Request a new doc build by commenting
If your PR continues to fail for an unknown reason, the doc build pipeline may be broken. Elastic employees can check the pipeline status here. |
|
buildkite test this please |
elasticsearch/helpers/errors.py
Outdated
|
|
||
| class ScanError(Exception): | ||
| scroll_id: str | ||
|
|
||
| def __init__(self, scroll_id: str, *args: Any, **kwargs: Any) -> None: | ||
| super().__init__(*args, **kwargs) | ||
| def __init__(self, scroll_id: str, shards_message: str) -> None: |
There was a problem hiding this comment.
Are the typing changes in both exeception classes and the argument changes in ScanError necessary? I think you could have just added a __reduce__ method that writes all the arguments assuming the caller is passing pickle-friendly data. Not a huge deal, but to me it seems there is no point in restricting the arguments in ScanError, given that it is a backwards-incompatible change.
There was a problem hiding this comment.
They're not necessary, but **kwargs was simply not working, and *args was overly broad. Restricting to what was actually used also really improved the types. Are you thinking that other libraries could be raising those exceptions with different parameters?
There was a problem hiding this comment.
https://github.com/search?q=%22raise+BulkIndexError%22+language%3APython&type=code shows that raising BulkIndexError() is pretty common, but I've only changed the type annotation here, and all the samples I've seen indeed pass a string.
I've not seen public examples of raising ScanError, but I can restore *args.
There was a problem hiding this comment.
I've restored backwards-compatibility in 0db50e4 (#2669), please take another look.
There was a problem hiding this comment.
It's highly unlikely that someone is raising ScanError, but yes, I just thought it was a change that is unrelated to making the class work with pickle. But as I said, it's a small observation, I think it is unlikely to cause problems.
|
buildkite test this please |
Co-authored-by: Quentin Pradet <quentin.pradet@elastic.co> (cherry picked from commit 08addf2)
Co-authored-by: Quentin Pradet <quentin.pradet@elastic.co> (cherry picked from commit 08addf2)
|
Thank you for your contribution, it is now released as part of 8.16.0: https://github.com/elastic/elasticsearch-py/releases/tag/v8.16.0 |
I encountered an issue with pickling/unpickling BulkindexError.
unpickle_exception\n inst = func(*args)\n ^^^^^^^^^^^\nTypeError: BulkIndexError.**init**() missing 1 required positional argument: 'errorsI'm adding an implementation of
__reduce__to this (and ScanError) so they can be pickled and unpickled correctly. Added tests for it as well.