-
Notifications
You must be signed in to change notification settings - Fork 13
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
fix(dgw): adjust delete_many endpoint #1095
Conversation
Instead of failing and returning 404 Not Found when one of the recordings is not found, we return the number of recordings not found in the response body with a 200 Ok. The client may use this information to notify the user about a possible out of sync state on their side and suggest to rerun an indexing operation. Changelog: ignore
@@ -88,6 +90,88 @@ export class JrecService { | |||
return httpParams; | |||
} | |||
|
|||
/** | |||
* Mass-deletes recordings stored on this instance | |||
* If you try to delete more than 1,000,000 recordings at once, you should split the list into multiple requests to avoid timing out during the processing of the request. The request processing consist in 1) checking if one of the recording is active, 2) counting the number of recordings not found on this instance. When a recording is not found on this instance, a counter is incremented. This number is returned as part of the response. You may use this information to detect anomalies on your side. For instance, this suggests the list of recordings on your side is out of date, and you may want re-index. |
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.
[nit] Something gone wrong with formatting here
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.
The generator is removing newlines, but I think it’s fine though.
let localVarHeaders = this.defaultHeaders; | ||
|
||
let localVarCredential: string | undefined; | ||
// authentication (scope_token) required |
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.
[question] Are we using the same style of comments here in ts
as in rust, or are we using a different convention? (Full sentences ending with .
)
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.
This is generated code, so we don’t follow anything on our side specifically
) -> Result<Json<DeleteManyResult>, HttpError> { | ||
use std::collections::HashSet; | ||
|
||
const BLOCKING_THRESHOLD: usize = 100_000; |
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.
[qeustion]: This threshold looks pretty big, what is the measured delay for processing 100000 items without spawn_blocking
?
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.
100,000 values are processed in under 100ms on my machine, for more than 100,000 we use spawn_blocking (the threshold is not 1,000,000).
Instead of failing and returning 404 Not Found when one of the recordings
is not found, we return the number of recordings not found in the
response body with a 200 Ok.
The client may use this information to notify the user about a possible
out of sync state on their side and suggest to rerun an indexing
operation.
Changelog: ignore