feat/labs migration#1968
Conversation
Part of kernelci#1948 Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
Part of kernelci#1948 Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
alanpeixinho
force-pushed
the
feat/labs-migration
branch
from
7ee4011 to
80d7ce6
Compare
* For analysis queries we are going for lab column information
first, and coalescing to json misc information.
* All COALESCE expressions are marked with TODO comments for removal
after the lab_id backfill is complete.
Closes kernelci#1948
Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
alanpeixinho
force-pushed
the
feat/labs-migration
branch
from
80d7ce6 to
3404f84
Compare
|
|
||
|
|
||
| def assign_lab_ids(builds_buf: list[Builds], tests_buf: list[Tests]) -> None: | ||
| """Resolve each instance's real lab name to a labs.id and set its lab_id FK. |
There was a problem hiding this comment.
If the lab name is unique in the database, why not use that as a primary key? In that case you'd make a first query to check which labs are not in the database yet, a second query to update the table if needed, but you wouldn't need the third query to get the id of the newly added labs
There was a problem hiding this comment.
Mostly to avoid string keys, since this is an internal key, we might benefit more from integer indices.
There was a problem hiding this comment.
Makes sense. I am worried that we are adding one query here, one query there and soon we will have a bloat of unnecessary queries in the ingester, but it's fine for now
| tests.number_value AS tests_number_value, | ||
| tests.misc AS tests_misc, | ||
| tests.environment_compatible AS tests_environment_compatible, | ||
| COALESCE(test_labs.name, tests.misc ->> 'runtime') AS tests_lab, |
There was a problem hiding this comment.
I think you forgot a TODO here
| ) | ||
| return list(result) | ||
| tests = [] | ||
| for test in result: |
There was a problem hiding this comment.
Why not assign directly to lab like before?
| result = [] | ||
| for row in rows: | ||
| build_misc = row[11] | ||
| sanitized_build_misc = sanitize_dict(build_misc) |
There was a problem hiding this comment.
Why does this not use the fallback?
| log_excerpt = models.CharField(max_length=16384, blank=True, null=True) | ||
| misc = models.JSONField(blank=True, null=True) | ||
| lab = models.ForeignKey( | ||
| Labs, db_constraint=False, null=True, on_delete=models.DO_NOTHING |
There was a problem hiding this comment.
Shouldn't we use db_constraint=True?
There was a problem hiding this comment.
I see all other tables are using db_constraint=False, so there must be a good reason for this, but I do not see it documented
There was a problem hiding this comment.
For the kci keys, we unfortunately have some missing keys in a few rows. But since lab has only internal keys, I believe we can safely enforce constraints on them. Good point.
| build_lab_summary = builds_summary.labs.get(lab) | ||
| if not build_lab_summary: | ||
| build_lab_summary = StatusCount() | ||
| builds_summary.labs[lab] = build_lab_summary |
There was a problem hiding this comment.
Copy pattern from previous chunk for consistence and readability
3 participants
Summary
Migrates all read paths for lab information from JSONB
miscfields (builds.misc->>'lab',tests.misc->>'runtime') to the newlabstable vialab_idforeign key, with JSONB fallback for rows not yet backfilled.Every SQL query and Python helper that previously extracted lab names by parsing JSONB now uses
COALESCE(labs.name, misc_fallback)through aLEFT JOINon thelabstable. This ensures:lab_idpopulated by the ingester) reads from the structured FKlab_id) still works via the JSONB fallbackTODOcomments)How to test
legacy database
lab_idis NULL on existing rowsMigrated database (test in a local database)