Implement S3 merge task status check - #66
Conversation
| struct spdk_lvs_merge_result *result = calloc(1, sizeof(*result)); | ||
| if (result == NULL) { | ||
| SPDK_ERRLOG("Cannot allocate memory for merge result s3_id %u old_s3_id %u; " | ||
| "status will be unobservable.\n", xfer->s3_id, xfer->old_s3_id); |
There was a problem hiding this comment.
when you cannot allocate memory for sure we have issue so no need to completed return fails
There was a problem hiding this comment.
Not sure what you mean here, should we fail the transfer?
|
|
||
| /* Count-based backstop alongside the time-based sweep in | ||
| * md_xfer_poller() — drop the oldest if it's ever starved. */ | ||
| if (g_lvs_merge_results_count > LVS_MERGE_RESULTS_MAX) { |
There was a problem hiding this comment.
No need to set Max entries and check the counter against it just increment it,
There was a problem hiding this comment.
What would we gain by dropping the defensive guard. It's cost is minimal, I can add a unlikely() on the condition to reduce it even further. In regular operation this will never trigger, but software tends to surprise us with unexpected edge cases so I'd like to be conservative here.
There was a problem hiding this comment.
bcs first I don't see reason for the limit
second idk what is the rate of merge request
| /* Sweep g_lvs_merge_results, rate-gated to roughly once a second — this | ||
| * poller already runs at a 1ms period for the process lifetime once any | ||
| * transfer has started, so this reuses it rather than adding a poller. */ | ||
| static uint64_t last_sweep_ticks = 0; |
There was a problem hiding this comment.
I like the idea of using a timeout, but in the end, I would prefer it to work like this:
For each merge task, the final call should check the task result and make sure the merge completed correctly. Once the result has been checked, you have a clear signal that the task result can be removed from the queue.
If management calls the task-status API and the task status is either failed or done, you can return the result to management and then remove the corresponding task object from the queue.
There was a problem hiding this comment.
This would make the check non-idempotent, and have us rely on the CP checking the status exactly once. With network issues, controller restarts etc this is not necessarily guaranteed. In conjunction with the comment above we that's an avenue for stale data in the list. I'd like this a bit more robust to misbehavior on the CP side, and give the CP more room for changes in design, if e.g. separate components check the merge status independently.
There was a problem hiding this comment.
not exactly once, why once? until its completed
1- if the task failed/done why should we receive new check stat? first check is enough
2- if we receive check stat for the task that is not there it will be same as what you do right now bcs you remove it after a time
3- in this case that you consider the time you can mix them after the task done keep your time
as I said I prefer this not necessary to be only the solution
if you think what you do is better than go ahead with it no objection here
2 participants
No description provided.