Skip to content
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(bpdm-orchestrator): introduced pagination for handling pending and retention timeouts #1029

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

SujitMBRDI
Copy link
Contributor

Description

This pull request introduces pagination for handling pending and retention timeouts in the orchestrator. By processing tasks in manageable batches, the changes prevent out-of-memory errors during the execution of large volumes of tasks, improving the system's stability and efficiency.

Pre-review checks

Please ensure to do as many of the following checks as possible, before asking for committer review:

@SujitMBRDI SujitMBRDI added the bug Something isn't working label Aug 12, 2024
@SujitMBRDI SujitMBRDI requested a review from nicoprow August 12, 2024 11:04
@SujitMBRDI SujitMBRDI force-pushed the fix/orchestrator-timeouts branch from 0a7cb97 to 950d1e0 Compare August 14, 2024 10:29
@nicoprow nicoprow added this to the BPDM v6.2.0 milestone Aug 15, 2024
@SujitMBRDI SujitMBRDI force-pushed the fix/orchestrator-timeouts branch from 950d1e0 to 6671d3a Compare August 19, 2024 04:37
Copy link
Contributor

@nicoprow nicoprow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic looks sound to me but I think using such an unspecifc public function in the golden record task service in which you can basically place in any fetch and process function to be executed seems to be quite over the top. Especially when we consider that the TimeoutProcessBatchService does have two very similar methods that just differentiate in between which they give delegates as parameters to that function in the golden record task service.

Why not just create two separate functions in the golden record task service, one for fetching and processing pending timeouts, one for fetching and processing retention timeouts. If you then want to reduce duplicate code you could create a private function for batch processing these functions.

@SujitMBRDI SujitMBRDI force-pushed the fix/orchestrator-timeouts branch from 6671d3a to 0e8329b Compare August 19, 2024 05:20
Copy link
Contributor

@nicoprow nicoprow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Details for my previous comments in the code

@SujitMBRDI
Copy link
Contributor Author

The logic looks sound to me but I think using such an unspecifc public function in the golden record task service in which you can basically place in any fetch and process function to be executed seems to be quite over the top. Especially when we consider that the TimeoutProcessBatchService does have two very similar methods that just differentiate in between which they give delegates as parameters to that function in the golden record task service.

Why not just create two separate functions in the golden record task service, one for fetching and processing pending timeouts, one for fetching and processing retention timeouts. If you then want to reduce duplicate code you could create a private function for batch processing these functions.

Yeah, will refactor code accordingly.

@SujitMBRDI
Copy link
Contributor Author

Details for my previous comments in the code

@nicoprow, i have adapted changes as suggested.

@SujitMBRDI SujitMBRDI force-pushed the fix/orchestrator-timeouts branch from fed9f73 to f3587bc Compare August 19, 2024 11:01
Copy link
Contributor

@nicoprow nicoprow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Minor suggestions in the comments

@nicoprow nicoprow assigned SujitMBRDI and unassigned nicoprow Aug 21, 2024
…d retention timeouts

fix(bpdm-orchestrator): updated changelog

fix(bpdm-orchestrator): introduced pagination for handling pending and retention timeouts

fix(bpdm-orchestrator): introduced pagination for handling pending and retention timeouts
@SujitMBRDI SujitMBRDI force-pushed the fix/orchestrator-timeouts branch from c1e1459 to 066617e Compare August 21, 2024 08:57
@SujitMBRDI SujitMBRDI merged commit 25e4c9c into main Aug 22, 2024
17 checks passed
@nicoprow nicoprow deleted the fix/orchestrator-timeouts branch August 26, 2024 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants