Cancellation as soon as possible of a generation that will fail#3621
Cancellation as soon as possible of a generation that will fail#3621sebr72 merged 2 commits intomapfish:masterfrom
Conversation
a46925c to
e879af8
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR improves the PDF generation process by ensuring that once one task fails, all tasks are cancelled promptly using a global cancellation flag. Key changes include:
- Passing a shared AtomicBoolean flag to indicate cancellation in various processor and attribute tests.
- Updating constructors and contexts (e.g., in AbstractProcessor and Values) to support the cancellation mechanism.
- Adjusting output formats and task execution to incorporate the new cancellation parameter.
Reviewed Changes
Copilot reviewed 75 out of 75 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/test/java/org/mapfish/print/processor/jasper/DateFormatProcessorTest.java | Updated test to include AtomicBoolean cancellation flag. |
| core/src/test/java/org/mapfish/print/processor/jasper/DataSourceProcessorTest.java | Injected AtomicBoolean cancellation parameter in processor test. |
| core/src/test/java/org/mapfish/print/processor/SetStyleProcessorTest.java | Added AtomicBoolean in test setup for style processing. |
| core/src/test/java/org/mapfish/print/output/ValuesTest.java | Modified tests to pass the cancellation flag via AtomicBoolean. |
| core/src/test/java/org/mapfish/print/map/tiled/SingleTileLoaderTaskTest.java | Adjusted context initialization to include the cancellation flag. |
| core/src/test/java/org/mapfish/print/map/geotools/grid/LineGridStrategyTest.java | Updated task creation to use AtomicBoolean cancellation flag. |
| core/src/test/java/org/mapfish/print/attribute/* | (Various attribute tests) Updated to pass the new cancellation flag parameter. |
| core/src/test/java/org/mapfish/print/AbstractMapfishSpringTest.java | Modified global test context to initialize with AtomicBoolean cancellation flag. |
| core/src/main/java/org/mapfish/print/processor/AbstractProcessor.java | Refactored Context to use AtomicBoolean for cancellation. |
| core/src/main/java/org/mapfish/print/output/Values.java | Updated Values constructors and methods to store and propagate the cancellation flag. |
| core/src/main/java/org/mapfish/print/output/MapExportOutputFormat.java | Injected the cancellation flag in output format task creation. |
| core/src/main/java/org/mapfish/print/output/AbstractJasperReportOutputFormat.java | Updated report generation to include AtomicBoolean cancellation flag. |
|
Looks good, what do you think @sebr72 |
|
@sbrunner I will have a look. And do some proper test before approving the PR. |
|
Hello, in my opinion, the copilot recommandations can be avoided. |
sebr72
left a comment
There was a problem hiding this comment.
I can see how you share the status CANCELED. but I am not sure how you validate it actually behaves like it is intended. Could you point out in the description of this fix which test would have failed in past,
Ideally if none satisfies you: please add one.
core/src/main/java/org/mapfish/print/processor/AbstractProcessor.java
Outdated
Show resolved
Hide resolved
74078b6 to
0e0c31d
Compare
|
Hello I have added a test (CreateMapPagesFailOnErrorProcessorTest.java) to prove the behaviour works normally. You can take the test without my correction and see that it fails. It works locally, i hope it works in your CI. The main improvement is related to the fact that when using the create map processor (with parameters that create multiple pages), it will passes to the datasource processor multiple input.values, and the datasource will create multiple tasks according to these values (see DataSourceProcessor.java function processInput, line 223 - 227) Before my modification all the tasks created by datasource processor had their own cancel boolean primitive object, so if one task fail, the other will not. Now they shared the same boolean object. Hope it's more clear for you. Tell me what you think, I have not the full architecture in mind, and I'm not expert on your project of course. |
sebr72
left a comment
There was a problem hiding this comment.
Very minor update. Otherwise everything was great.
core/src/test/java/org/mapfish/print/processor/map/CreateMapPagesFailOnErrorProcessorTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/mapfish/print/processor/map/CreateMapPagesFailOnErrorProcessorTest.java
Outdated
Show resolved
Hide resolved
0e0c31d to
bb152f7
Compare
|
@arnaudboudier-sogelink github is reporting 2 commits with the same comment. I guess it is an issue with the force push ? |
|
I just push the correction with the same text commit, if you decide to merge it, i sugess you squash them into one single commit. I can do it if you want |
I will use the squash commit as suggested by comiter.
* Cancellation as soon as possible of a generation that will fail * Cancellation as soon as possible of a generation that will fail
Using failOnError parameter when generating a PDF using paging with a big areaOfInterest, this will create a lof of tasks that will perform WMS requests in parallel for example.
The problem is that if for some reason, some WMS requests fail in one task, only the programed WMS requests of this current task will be cancelled. Other task will continue to perform WMS requests, even though at the end the pdf generation will fail because failOnError is activated.
The modifications done is this commit, make all the tasks share a global AtomicBoolean indicating if one task has fail. If so it will cancel all the tasks quickly. This can save generation time.