fix(risk): improve reprocess deployment risk#16987
Conversation
|
Images are ready for the commit at 2078c69. To use with deploy scripts, first |
|
Rebase to integrate #16983 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #16987 +/- ##
=======================================
Coverage 48.80% 48.80%
=======================================
Files 2696 2696
Lines 201446 201452 +6
=======================================
+ Hits 98310 98316 +6
- Misses 95378 95380 +2
+ Partials 7758 7756 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
31d6324 to
b515b59
Compare
|
This change is part of the following stack: Change managed by git-spice. |
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
b515b59 to
2078c69
Compare
|
/retest |
|
Caution There are some errors in your PipelineRun template.
|
|
You can merge this PR without my approval. I would like to see a more detailed explanation of how this PR was tested. What was the load used for the test. Is one screen shot with these changes and the other without? How were the screen shots created? |
dashrews78
left a comment
There was a problem hiding this comment.
Blocking so we don't merge this yet. I think we should go with my stack as the my changes to the query will help both central and central-db memory. My changes should also hold up better when auto locking is enabled. Or we can discuss that. I'm close on getting the migration done.
#16965 is my target implementation and a pattern for other places where we get large objects when we only need a few items.
Perhaps the implementation in this PR and @dashrews78 idea are not mutually incompatible. Though, it might not be easy to do both. |
|
Unit tests could be added to improve code coverage. |
The select framework would have to be updated. We probably should do that but that is larger. The benefit of my approach is that it helps the database too which we've been seeing have some issues with buffers in the larger environments. So the less we pull in the query the better. Once auto locking goes in, the approach in this PR won't save too much on the database side. |
|
I run benchmark that was in the package: go test -tags sql_integration -run=NO -bench=BenchmarkEvaluateBaselinesAndPersistResult ./central/processbaseline/evaluator/. -count 6 -benchmem -benchtime 3s -timeout 1h > new.txt \
&& git co - && \
go test -tags sql_integration -run=NO -bench=BenchmarkEvaluateBaselinesAndPersistResult ./central/processbaseline/evaluator/. -count 6 -benchmem -benchtime 3s -timeout 1h > master.txt
I'm surprised by low p value and huge deviation. Is it possible this benchmark does not cleanup the data and each run reads more and more data?
I totally agree although this PR is working and provides some relief on allocs and GC. In the long run we should avoid getting slices from database unless it's necessary. In this case it's not. |
My competitor PR is ready for review and also working. Long running cluster is running now. That's why I blocked this one because they work some what differently and would have conflicts. I think broad strokes it is one or the other and then we iterate on top. |
|
I slightly changed benchmark to collect max heap objects and bytes as this will differ the most and allocs will remain the same. |
|
Closing as it's needs to be implemented from scratch as in postgres we have no callback version of that query. |
SearchRawProcessIndicatorscalls are not optimal in our use case where we need to loop over elements of the process indicators return set. Instead let's use a callback approach. This will reduce memory and GC pressure as we do not need to load all objects into a slice but iterate one by one.go test -tags sql_integration -run=NO -bench=BenchmarkEvaluateBaselinesAndPersistResult ./central/processbaseline/evaluator/. -count 6 -benchmem -benchtime 3srefs: