fix Work recalculate concurrency with cascade and fix rd work - #210
Conversation
|
I haven't done a full review, but it looks very good and should close #171 if/when merged. |
| SPLIT("split"), | ||
| USER_INPUT("user"); | ||
| EDIVISIVE("ed",true), | ||
| FINGERPRINT("fp",true), |
There was a problem hiding this comment.
Why finger print node is marked as analysis node along with detection node? Asking since it is used for aggregating. Is it because detection nodes need them for grouping?
There was a problem hiding this comment.
An analysis node is one that analyzes the output of other nodes and does not make a node ephemeral.
There was a problem hiding this comment.
far as I've checked setActiveNodes() is not called but should this also requires a clean up here?
There was a problem hiding this comment.
I've checked setActiveNodes() is not called
Two things happened here. Work was previously a Panache Entity. Panache allows developers to write code that references public member variables and it will replace those references with getters and setters at compile time. This should have been fixed when Work was changed to a plain java object (POJO) but good catch that it wasn't performed. I fixed Work to no longer have public member variables and updated the PR.
also requires a clean up here?
When you say "clean up" it leaves me unsure of what you mean. I assume you mean that RelativeDifference should be removed from the check to make the Work cumulative and if so you're correct. I updated the method.
| NodeEntity found = NodeEntity.findById(node.id); | ||
| found.operation = ".node + 1"; | ||
| found.persist(); | ||
| System.out.println("found="+found); |
There was a problem hiding this comment.
Should there be proper asserts for the test?
There was a problem hiding this comment.
The omg test was something I created to test while making code changes but was not meant to be included in the PR. I removed it
| assertFalse(result.getOutput().contains("null")||result.getOutput().contains("NULL"),"list values should not contain null\n"+result.getOutput()); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Do we have a CLI test for per node re calculation?
There was a problem hiding this comment.
No, this PR was to correct the functionality in how work is handled. Once this is merged we can explore if it needs to be exposed to users via a CLI or if it should trigger automatically when users perform related actions (e.g. adding or updating a node).
e1d4b0f to
827e68e
Compare
Rebase aesh branch on origin/main. Update Quarkus version from 999-SNAPSHOT to 3.38.0 with aesh 3.16.4. Fix Vert.x Buffer import changes (core → mutiny). Make CLI command fields package-private to avoid reflective access warnings. Convert remaining picocli commands to aesh: AddEDivisive, ListProcessing, RemoveProcessing, ResumeProcessing. Create standalone AddFingerprint command (replaces --fingerprint flag on AddJq/AddJs). Fix test command syntax (node jq → node add jq). Remove outdated null-value assertion for ephemeral-nullified intermediate nodes. RecalculateCmd placeholder — folder-level recalculate API was removed by PR Hyperfoil#210, needs reimplementation for CLI. Detection test failures remain — fingerprint and fixedthreshold nodes produce correct structure but detection values are not generated. Requires investigation into work queue completion in REPL test context.
Rebase aesh branch on origin/main. Update Quarkus version from 999-SNAPSHOT to 3.38.0 with aesh 3.16.4. Fix Vert.x Buffer import changes (core → mutiny). Make CLI command fields package-private to avoid reflective access warnings. Convert remaining picocli commands to aesh: AddEDivisive, ListProcessing, RemoveProcessing, ResumeProcessing. Create standalone AddFingerprint command (replaces --fingerprint flag on AddJq/AddJs). Fix test command syntax (node jq → node add jq). Remove outdated null-value assertion for ephemeral-nullified intermediate nodes. RecalculateCmd placeholder — folder-level recalculate API was removed by PR Hyperfoil#210, needs reimplementation for CLI. Detection test failures remain — fingerprint and fixedthreshold nodes produce correct structure but detection values are not generated. Requires investigation into work queue completion in REPL test context.
Rebase aesh branch on origin/main. Update Quarkus version from 999-SNAPSHOT to 3.38.0 with aesh 3.16.4. Fix Vert.x Buffer import changes (core → mutiny). Make CLI command fields package-private to avoid reflective access warnings. Convert remaining picocli commands to aesh: AddEDivisive, ListProcessing, RemoveProcessing, ResumeProcessing. Create standalone AddFingerprint command (replaces --fingerprint flag on AddJq/AddJs). Fix test command syntax (node jq → node add jq). Remove outdated null-value assertion for ephemeral-nullified intermediate nodes. RecalculateCmd placeholder — folder-level recalculate API was removed by PR Hyperfoil#210, needs reimplementation for CLI. Detection test failures remain — fingerprint and fixedthreshold nodes produce correct structure but detection values are not generated. Requires investigation into work queue completion in REPL test context.
3 participants
This introduces Work.cascade which prevents work from creating more work when it creates new Values. This option is used to recalculate ephemeral Values for recalculate without incurring the performance penalty of wasted additional computation for unnecessary nodes and sorting.
This also fixes recalculate to work concurrently with upload and other recalculates that could impact the same scope. The RecalculateWorkQueueTest uses a WorkExecutor without threads to manually verify the queuing.
This PR also removes recalculate for a full folder.