Louis/enrichment datamodel #377
Conversation
ldelossa
force-pushed
the
louis/enrichment-datamodel
branch
from
5b27cb8
to
6d784f8
Compare
ldelossa
requested review from
alecmerdler,
hdonnay and
jzelinskie
as code owners
ldelossa
force-pushed
the
louis/enrichment-datamodel
branch
3 times, most recently
from
913a22d
to
1c2238c
Compare
| var uo driver.UpdateOperation | ||
| err := rows.Scan( | ||
| &uo.Ref, | ||
| &uo.Updater, | ||
| &uo.Fingerprint, | ||
| &uo.Date, | ||
| ) | ||
| if err != nil { | ||
| rows.Close() | ||
| return nil, fmt.Errorf("failed to retrieve update operation for updater %v: %w", updater, err) | ||
| } | ||
| ops := []driver.UpdateOperation{} | ||
|
|
||
| getUpdateOperationsCounter.WithLabelValues("query").Add(1) | ||
| getUpdateOperationsDuration.WithLabelValues("query").Observe(time.Since(start).Seconds()) | ||
|
|
||
| for rows.Next() { | ||
| ops = append(ops, driver.UpdateOperation{}) | ||
| uo := &ops[len(ops)-1] | ||
| err := rows.Scan( | ||
| &uo.Ref, | ||
| &uo.Updater, | ||
| &uo.Fingerprint, | ||
| &uo.Date, | ||
| ) | ||
| if err != nil { | ||
| rows.Close() | ||
| return nil, fmt.Errorf("failed to scan update operation for updater %q: %w", u, err) | ||
| } | ||
| } | ||
| rows.Close() | ||
| if err := rows.Err(); err != nil { | ||
| return nil, err | ||
| return nil, fmt.Errorf("failed to scan update operation for updater %q: %w", uo.Updater, err) | ||
| } | ||
| out[u] = ops |
There was a problem hiding this comment.
I'm a little worried about excess copying doing it this way, but it's certainly simpler to read.
There was a problem hiding this comment.
Where are the concerns?
There was a problem hiding this comment.
The previous version allocates a local slice, grows it and populates in-place, then adds it to the map, so the slices in the map never need to be updated once they escape. This version allocates a uo local, populates it, then potentially reallocates the updater's slice copies it into the slice.
| if err := tx.Commit(ctx); err != nil { | ||
| return nil, fmt.Errorf("failed to commit transaction: %w", err) |
There was a problem hiding this comment.
I see why we wouldn't bother with the commit, but it's a little weird. Using BeginTx with a read-only option would call out this being read-only in code.
There was a problem hiding this comment.
Yeah I'm not sure why we went with a transaction here to begin with, I think we can get rid of it and make this a read only method all together, right?
There was a problem hiding this comment.
I thought the transaction was needed to make sure the same snapshot of the table is used, since we're issuing multiple queries.
ldelossa
force-pushed
the
louis/enrichment-datamodel
branch
from
1c2238c
to
06ef7fd
Compare
This commit adds the necessary migration to support the Enrichment feature per out specification: https://github.com/quay/clair-enrichment-spec/ This commit will be backported to stable branches to maintain upgrade path compatibility. Signed-off-by: ldelossa <ldelossa@redhat.com>
ldelossa
force-pushed
the
louis/enrichment-datamodel
branch
from
06ef7fd
to
468a76f
Compare
this commit introduces the necessary changes to support the enrichment specification's data model changes. see: https://github.com/quay/clair-enrichment-spec/ In addition to this, a small bug was fixed in the updater code where errors were not being logged.
hdonnay
force-pushed
the
louis/enrichment-datamodel
branch
from
468a76f
to
9a3b349
Compare
Draft pull request to implement Enrichment data model changes per:
quay/clair#1229