Switch back to botocore _make_api_call under the hood #1079
Conversation
pynamodb/connection/base.py
Outdated
| @@ -349,124 +349,7 @@ def send_pre_boto_callback(self, operation_name, req_uuid, table_name): | |||
| log.exception("pre_boto callback threw an exception.") | |||
|
|
|||
| def _make_api_call(self, operation_name: str, operation_kwargs: Dict, settings: OperationSettings = OperationSettings.default) -> Dict: | |||
There was a problem hiding this comment.
are we concerned about settings: OperationSettings now not being used? I see it only passed into self._create_prepared_request
There was a problem hiding this comment.
ah thats probably TODO Reimplement extra_headers (through the before-sign. hook)
There was a problem hiding this comment.
The motivation for 'settings' was to allow per-request controls of retries, e.g. you might want to hedged-retry a read but not a write. Honestly didn't end up using this anywhere. With retries delegated to boto, this might not be needed since we'd no longer retry perm-fails indiscriminately.
There was a problem hiding this comment.
I removed this in 4f28c9f since I can't find a good way to retain this without taking more deps on botocore internals, and I think it's unlikely to be used much
|
+1 |
garrettheel
force-pushed
the
move-to-botocore-make-api-call
branch
from
a5a9c6a
to
673137d
Compare
|
Update on this: the performance regression was larger than I expected, so I don't feel comfortable merging this as is. While I was able to get back a chunk of that by removing additional response parsing (~5%), there is still another 20% or so regression that comes from a lot of little things and is not easily fixed (hooks, passing around context for retries, etc.). I'll leave this open for now in case something changes later that makes this worth revisiting. At least the benchmark script is likely to come in handy in future |
Improving the first iteration logic in `PageIterator` and `ResultIterator`. Both iterators had a `_first_iteration` state attribute that could be replaced with better logic: - In `PageIterator`, it can be replaced with `_is_last_page` which - is exception-safe (only mutate after we do I/O), and - (IMO) more directly relates to the state we're handling - In `ResultsIterator`, we can do without `_first_iteration` (but we should initialize `_index` and `_count`) Inspired by #1059.
|
@garrettheel is this still in the works? |
|
@ilanjb not right now, the performance regression of 25% is a bit too high. this could be revisited if botocore performance improves and lessens the delta |
test.py
Outdated
| @@ -0,0 +1,49 @@ | |||
| import traceback | |||
What change is that? FWIW, I'm feeling like we should merge this for sanity/correctness, and then perhaps plead botocore to optimize? (Are there any obvious bottlenecks we can point them at?) |
ikonst
force-pushed
the
move-to-botocore-make-api-call
branch
from
f046855
to
214c690
Compare
ikonst
force-pushed
the
move-to-botocore-make-api-call
branch
from
214c690
to
69de228
Compare
|
Fixed conflicts with master. Given the resourcing to maintain this project and how deeper uses of internal botocore APIs made us rush with hot fixes last year, I think the 20% slowdown is a good tradeoff. |
That's fair, I'm on board with merging if you like. We should just make sure to update the README/project motivation accordingly and be pretty upfront in the release notes |
It's not clear that the reasons for diverging from botocore's
_make_api_callare still valid 7 years later. The most immediate negative impact of this implementation is that PynamoDB is incompatible with the opentelemetry botocore instrumentation.Our retry implementation is also lacking compared to botocore's these days, so we'll be better off by switching back.
Note that this PR lessens the degree to which we're using internal private methods in botocore, but does not eliminate it entirely.
TODO:
base_backoff_ms(not valid when using native botocore retries)VerboseClientErrorwrapping)before-sign.hook)Note: I also modernized the connection/base test since it was the most affected by this change.
Benchmarks