MPL-68: make run_schemas updates asynchronous #1581
Conversation
There was a problem hiding this comment.
Added a couple of tests, not sure if this is enough but at least it is a start.
There was a problem hiding this comment.
Do the tests at least cover the use cases that caused the issue?
There was a problem hiding this comment.
It depends what you mean by "the issue", because actually the issue (i.e., the timeout) happens only with a large number of runs that we don't have in test environment.
Moreover here we are not actually solving the issue (the timeout), but we are just moving the run_schemas update asynchronously and the tests cover both the creation and update of a Schema, checking if the run_schemas table is eventually updated.
There was a problem hiding this comment.
There are a couple of areas that I think we need to resolve.
This change also requires changes to production configuration. Ideally we would want an update to an "admin guide" (that we don't have: #1438). I don't think admin is a blocker, but have linked here to reference when we do create an admin guide
horreum-api/src/main/java/io/hyperfoil/tools/horreum/api/data/Schema.java
Outdated
Show resolved
Hide resolved
horreum-backend/src/main/java/io/hyperfoil/tools/horreum/svc/ServiceMediator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Do the tests at least cover the use cases that caused the issue?
lampajr
force-pushed
the
MPL-68_on_schema_async
branch
from
d9296e5 to
2a5aa2f
Compare
|
As suggested here I added a (timeout) banner notifying the user that run-schema links might not be reflected immediately: I decided to go with a timeout banner rather than a fixed banner (saved in the db) as I think we would like to show that banner only after schema save (and for a limited amount of time). |
| } | ||
|
|
||
| @org.junit.jupiter.api.Test | ||
| public void testCreateSchemaAfterRun() throws InterruptedException { |
There was a problem hiding this comment.
Should we mention that the run is an array run in the test name? The current Horreum implementation handles arrays and json objects in a significantly different manner.
Maybe testCreateSchemaAfterRunUsingArrayData?
There was a problem hiding this comment.
yeah you are right, I created two additional test cases and used a more specific method name for all of them
| Util.withTx(tm, () -> { | ||
| em.clear(); | ||
| List<?> runSchemas = em.createNativeQuery("SELECT * FROM run_schemas WHERE runid = ?1").setParameter(1, runId).getResultList(); | ||
| // two records as the run is an array of two objects, both referencing the same schema |
There was a problem hiding this comment.
This comment escaped the copy, paste, edit :)
|
With latest commit 5e9e17a I added a couple of tests covering other slightly different use cases as suggested #1581 (review). If you guys agree on this and this is ready to be merged, I will squash all commits before merging. |
|
@willr3 please can you review this PR again after the update? |
Thanks Will, I am going to squash all commits then |
lampajr
force-pushed
the
MPL-68_on_schema_async
branch
from
5e9e17a to
e30e650
Compare
Fixes Issue
Fixes #1579
Changes proposed
Changes:
schema-sync-{in, out}and related eventCreateOrUpdateEvent(TBH I think this could be simplified further as we just need theschemaIdvalue, thus we can remove the event and keep an id only)newOrUpdatedSchemafromServiceMediatortoSchemaServiceImplfor coherence (especially as we need the TxManager)runService.onNewOrUpdatedSchema(event.id);asynchronousCheck List (Check all the applicable boxes)