Core data: Fall back to default API batch size of 25. #39524
Conversation
|
Size Change: +82 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
|
AFAIR the default maxItems value may be updated on the backend (filters perhaps?). If we then default to 25 on the frontend, the two may disagree. Cc @TimothyBJacobs for a confidence check |
|
Correct, if |
Part of #39211 Previously it was possible that we fail to register a batch size from the server and we leave the batch size null. In this patch we fallback to the Core-defined default of 25.
dmsnell
force-pushed
the
core-data/fix-typing-issues-default-processor
branch
from
5c1c9a9 to
f930527
Compare
|
Thanks to both of you for reviewing this PR.
That's correct, and it's the purpose of the code in this PR to ask the backend what that value is. The problem was that if the request to fetch the batch size failed then all requests for that batch operation would never get sent out. Instead of sending out those requests the code inside of My effort around setting a default is more for situations where we're unable to get that configured batch size. I'd rather send out something that seems like a reasonable default (the default that PHP uses when no plugins adjust it) than to give up entirely. The other option we have would be to send out all the requests in parallel, possibly by grouping them in parallel chunks or at-most-N-at-a-time. Given that I couldn't find any reported problems with this I don't want to spend much effort on it, but I also found it hard to just pass by knowing there's this latent bug in here where API requests could suddenly stop processing.
I've updated the patch to preserve the error but also to get it out of the way of the execution loop so we can continue to process requests using the default value. If we're not keen on this I can see a couple other options for the general handling of this value.
I'm in favor of the second option because it's simpler still and doesn't need to halt for an error to fall back to the default. The downside to the approach is that we could send a larger batch size than some plugin has decided to limit the server. If that happens then we would expect the entire batch to fail. Maybe some client-side code would try again and have the actual value for We could also set the value low, like Again though, I don't want to spent too much time on a problem that isn't a known problem. If nobody likes any of these proposed ideas then we can hard-code the type-error into the system and ignore this latent bug for now. |
|
Closing for now due to lack of interest and review. We can readdress if we want to close this gap. I think there's still something valuable to consider here since we are delaying sending batched requests until we hear back on how many can be accepted at once. |
@dmsnell there is an entire rabbithole of context on that:
tl;dr:
Of course, batches are not quite "atomic" in the same way as a database transaction is, but they have some useful mechanics like an all-or-nothing validation. I'd rather not introduce a case where the requests are unexpectedly "unbatched" and sent over one by one.
I think this would be the best way forward. The worst-case scenario seems to be that the batch size sent exceeds the The only caveat is that I'd add an opt-out switch here for the cases such as:
...or, thinking about it more, it would be even better to make the |
Holy coconuts batman, this is a frightening thing to read, that people are using the batch endpoint as a pseudo-transaction.
Can't this still happen with normal batching requests within the allotted batch size? there's nothing that makes those requests succeed or fail together, and to the contrary there's logic in there to record the success or failure of individual requests.
Part of the reason for closing this PR is just that I feel this has warranted more complexity than it deserves already. In fact, if it weren't for the fact that we're injecting latency into the first batch request by waiting on that query (and then also possibly shutting down all batch requests if it fails) then I wouldn't have addressed it. I would have expected at least an implicit constraint of a batching system to be that there's no guarantee of processing all the requests together. Surprised we're using it to avoid creating an endpoint we know we need. |
It's used in the widgets and the navigation editors. It used to be a bunch of parallel / sequential requests and the batch is already a big improvement on the way to actual atomic operations. Ideally this would be a real MySQL transaction with actual commit and rollback mechanics, but it's unclear how to rollback things like calls to WordPress hooks – especially when there are caches listening to them. We may get there at some point, but it's a process. See the discussions I linked to above for more context.
It can still happen and yes, it worries me too. Batches don't solve the problem fully, they just eliminate some of the possible failures like partial network errors, or partial validation problems.
This constraint exists, e.g. when you exceed the max batch size. I'm just hesitant to default to sending the requests one by one.
I'd love to have such an endpoint, I even had an exploratory PR going. It proved to be more involved than it initially seemed and discontinued at a time, but I believe that transactionality could be achieved given enough effort. |
yeah this discussion can happen elsewhere but the problem is not being unable to perform DB transactions or rollback API calls; it's bound to be the result of duplicating server logic into an array of client-side imperative APIs. that doesn't mean building a necessary API is easy; it just means that a batch API kicks the can a little further down the road but leaves the problem unchanged. that is, it doesn't just not "solve the problem fully," it leaves the problem unaddressed. that can have value, but we should be careful about communicating that a batch API can do what it can't, and if people are leaning on it for that then it could be useful to put pressure against that behavior otherwise we entrench it as an unlisted expectation which will break things in a very confusing way once that day comes :) |
@dmsnell I think we mean the same thing, only approaching it from two different angles:
The latter largely doesn't happen now. Is it bulletproof? No, there are cases in which the half-broken state will still happen. I believe the rollbacks are necessary to make any significant progress on these. |
What
Part of #39211
Previously it was possible that we fail to register a batch size from
the server and we leave the batch size null.
In this patch we fallback to the Core-defined default of 25.
Testing Instructions
It may not be possible to test this as it probably can never occur that we get to the point in line 39 without having set
maxItems.Maybe we should type-cast instead to assert that
maxItemsis set? But it's just a tiny addition as a compromise to prevent playing mind tricks in the type system.