ssl-hep / ServiceX_frontend

Client access library for ServiceX
https://servicex-frontend.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
5 stars 11 forks source link

Resume request when client fails fixes #468 #494

Closed ketan96-m closed 1 month ago

ketan96-m commented 1 month ago

Resolves #468

Problem

When the client submits the request but encounters client failure, there is no way of resuming the transforms from the last point. The client is forced to rerun the request which triggers the backend again.

Fix

This PR fixes the broken client problem. The client will be able to resume the request from the last point. Given the client submits the same request again (important since the request hash is calculated and compared with any pending requests) Future scope:

Approach

Created a new instance of tinydb whose sole purpose is to keep track of the transform request submissions. I had a discussion with @ponyisi whether we should have the cache use additional columns and flags. The cache currently holds the TransformationResults AFTER the request is processed. We could technically have additional columns, but it would mean changing previous cache specific functions. I propose to create a new instance of tinydb which stores the submitted transform request ( I have named it as queue). This will allow to add transform request submission related functions to be separate from the cache features. Thus keeping the existing code intact and only adding the necessary steps in between. Also this will allow us to add the future scope (CLI) option without much modification.

Open to design inputs

Details of this PR

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.11%. Comparing base (7a65b87) to head (40ad11d). Report is 10 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #494 +/- ## ========================================== + Coverage 83.10% 84.11% +1.01% ========================================== Files 26 26 Lines 1415 1505 +90 ========================================== + Hits 1176 1266 +90 Misses 239 239 ``` | [Flag](https://app.codecov.io/gh/ssl-hep/ServiceX_frontend/pull/494/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ssl-hep) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/ssl-hep/ServiceX_frontend/pull/494/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ssl-hep) | `84.11% <100.00%> (+1.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ssl-hep#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ketan96-m commented 1 month ago

@ponyisi @BenGalewsky @gordonwatts ready for review

BenGalewsky commented 1 month ago

I'm sorry, @ketan96-m - I don't agree with @ponyisi on this... adding a new database just to avoid updating existing code is just a recipe for technical debt. Please add a new boolean column for completed which indicates if this request has reached an end one way or another. That way you can add entries as soon as they are submitted.

ponyisi commented 1 month ago

Superseded by #497