mikebridge commented on PR #45532: URL: https://github.com/apache/superset/pull/45532#issuecomment-6321085103
@fitzee thanks for the review, and for the worked example on retries. **1. Retrying after a timeout.** Fixed in 1668b63f6f, with an extra test for the second check in 496dfd8528. `CreateSemanticViewCommand` now checks uniqueness before the provider lookup, and checks again just before the insert so the check-to-insert window stays as short as it was in #45287. A view that already exists is refused without a lookup, so resending the whole batch spends the budget only on the views that are still missing. The new test `test_post_semantic_view_retry_after_timeout_adds_remaining_views` runs the real command against a fake clock: the first request adds two views and times out on the third, and the retry adds the third with one lookup. It fails on the previous head with a 422. The command tests that pinned the old lookup-then-check order were updated, and the docs now say that resending the whole request is safe. I also merged master, which brings in #45524: the modal marks created views as added and drops them from the selection after a partial success, s o the modal no longer resends them either. The server-side fix still matters for other API clients. **4. Deadline boundary.** Done in the same commit. The mocked lookup now advances the clock by exactly the budget, so changing `>=` to `>` fails the test. I checked that mutation locally. **2. Top-level JSON array returns 500.** You're right that the handler raises on it. I tried the one-line `isinstance(body, dict)` guard, but the request still returns 500: the shared event logger then fails in `collect_request_payload` (`superset/utils/log.py:141`, `payload.update(json_payload)`), which raises `ValueError: dictionary update sequence element #0 has length 1; 2 is required`. That affects every endpoint that receives a top-level array, so I'd rather fix both in a separate PR than make this one touch shared logging. **3. `None` or negative config values.** I've left these as they are. Both settings are typed as numbers, and `0` behaves as documented: `MAX_VIEWS = 0` refuses every batch and `TIMEOUT = 0` times out every view, which is how the tests run the all-timed-out case. I'd rather not add an "unlimited" `None`, because removing the bound is what this PR is trying to prevent. **5. Identifying timed-out views.** Agreed that it's ambiguous for a client that sends one name with two configurations. I've left it for now because the modal sends one configuration per request. Echoing the request index would be a small follow-up if an API client needs it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
