Add v2 matviews to nightly refresh #2667

rsanheim · 2021-06-29T22:48:58Z

Story card: ch3892

In my dev environment, with a large dataset, the new mat views take 20 minutes to refresh...so its pretty long for dev, but not unexpected given this data set size:

}
[2021-06-29T22:11:45.408-05:00] DEBUG:    (308.9ms)  COMMIT
[2021-06-29T22:11:45.409-05:00] INFO: refresh_matviews.all_v2 (1388043.2ms)
{

[1] pry(main)> Patient.count
=> 516,441
[2] pry(main)> BloodPressure.count
=> 1,867,157

shortcut-integration · 2021-06-29T22:49:00Z

This pull request has been linked to Clubhouse Story #3892: Wire up controlled & uncontrolled reports from pipeline via a toggle.

ssrihari

The metrics/measurement looks good. Just added some notes about refreshing the new matviews.

ssrihari · 2021-06-30T10:40:58Z

app/services/refresh_materialized_views.rb

+    ReportingPipeline::PatientBloodPressuresPerMonth
+    ReportingPipeline::PatientStatesPerMonth
+    ReportingPipeline::PatientVisitsPerMonth
+  ].freeze


@rsanheim We would only need:

ReportingPipeline::PatientStatesPerMonth.refresh

The cascade: true in the scenic refresh function takes care of refreshing the dependent views.

Cascade doesn't work, and it relies some of its own SQL parsing and regexp that I wouldn't really trust even if it did work for our case 😄 . I'll make sure to remove all the cascade: true options for the new matviews.

I think its fine to know what our dependency chain is and declare it explicitly in the refresh object...its a pretty key part of these matviews and we shouldn't be refreshing them in isolation for most cases anyways. New tests that rely on the matviews can call refresh_v2 for their setup, the same way we had a repeated refresh call for the old ones.

Oh well. Thanks for catching that btw! 😅

ssrihari · 2021-06-30T10:43:13Z

app/models/reporting_pipeline/patient_blood_pressures_per_month.rb

+    self.table_name = "reporting_patient_blood_pressures_per_month"
+    belongs_to :patient
+  end
+end


Can we avoid adding this model until we actually use it in code? I was even trying to avoid adding the Visits model, but we wanted to unit test that finely. This model's unit tests are already covered in the States model's unit tests.

I was doing that initially, but then I discovered the cascade issue in testing (see https://github.com/simpledotorg/simple-server/pull/2667/files#r661684079) and using this all locally...and I didn't want to add a manual refresh call for just one of our views.

We could also just call scenic's refresh with a given table name without creating a model. I was doing this before I saw the cascade option.

ssrihari · 2021-06-30T10:44:31Z

app/services/refresh_materialized_views.rb

-        MaterializedPatientSummary.refresh
+  def refresh_v2
+    ActiveRecord::Base.transaction do
+      ActiveRecord::Base.connection.execute("SET LOCAL TIME ZONE '#{tz}'")


ReportingPipeline::Matview.refresh already takes care of the transaction and the timezone setting. We can just wrap with metrics here.

The outer transaction is to rollback if any part of this fails I think, which is why we are doing it w/ the other views as well.

The tz piece is also just for safety and consistency w/ the other refresh...the more I think about this the more I think we should just move all the refresh logic here and not have it in the model objects at all. It doesn't make sense to refresh a matview in isolation, as they are all pretty tightly coupled w/ each other.

I actually think it's better to express the dependency tree closer to the model where there's most context about it. Each view can express its own dependencies, that way the refresh logic is in one place. When we change it for tests, we change it for the job as well.

I mean, you can't really see the dependency tree unless you look at the SQL itself. Abstractions on abstractions here. 😁

I actually think it's better to express the dependency tree closer to the model where there's most context about it. Each view can express its own dependencies, that way the refresh logic is in one place.

I can't think of a way to do that that would be more clear than a method that calls the refresh methods in the order we need, the dependency ordering is pretty clear there. Suggestions welcome though.

I just meant having the same list in the patient-state model itself. I'll raise a tiny PR for this perhaps.

See scenic-views/scenic#275

Responded to all feedback - we need to handle the cascade ourselves

…ple-server into add-v2-matviews

kpethtel · 2021-06-30T19:13:14Z

spec/models/reporting_pipeline/patient_states_per_month_spec.rb

@@ -19,7 +19,7 @@
        patient_registered_13m_ago = Timecop.freeze(13.months.ago) { create(:patient) }
        Timecop.freeze(13.months.ago) { create(:blood_pressure, patient: patient_registered_13m_ago) }

-        described_class.refresh
+        RefreshMaterializedViews.new.refresh_v2


Hopefully this isn't too much slower in specs than just refreshing the described class.

…ple-server into add-v2-matviews

ssrihari · 2021-07-01T06:01:19Z

spec/models/reporting_pipeline/patient_states_per_month_spec.rb

+    Timecop.freeze("June 30 2021 5:30 UTC") do # June 30th 23:00 IST time
+      example.run
+    end
+  end


The issue here is in using relative times. This was always the case, perhaps the reporting tables make the issue more apparent.

My preference would be to use the absolute times for the test as in june_2021 that we setup in the helper, as opposed to relative times.

My preference would be to use the absolute times for the test as in june_2021 that we setup in the helper, as opposed to relative times.

Yeah, I definitely prefer absolute times as well. I think the point I was trying to make is that even with the june_2021 helper, we need to establish an absolute time via freeze, or things will fail like this intermittently.

This spec does use june_2021 helper btw, but that doesn't help when time is a frustrating continually advancing thing. 😃

Hrm, I'm not sure I follow. What are the timestamps in concern that are advancing? Is it the time at which refresh runs that's the issue?

Hrm, I'm not sure I follow. What are the timestamps in concern that are advancing? Is it the time at which refresh runs that's the issue?

The timestamps that are advancing is system time, i.e. Time.current ...without the Timecop.freeze, we would have intermittent failures in this spec depending on the date and time the spec is being run.

rsanheim added 4 commits June 29, 2021 17:04

Start adding new mat views

408c186

Add BPs per month

96eb6cc

Refactor and add specs for v2

d35e6d4

freeze

48630b6

rsanheim requested a review from a team June 29, 2021 22:49

linting

7d0d611

rsanheim had a problem deploying to simple-review-pr-2667 June 29, 2021 23:59 Failure

ssrihari previously requested changes Jun 30, 2021

View reviewed changes

Turn cascade off, it refreshes too many tables

f4a4db5

See scenic-views/scenic#275

rsanheim temporarily deployed to simple-review-pr-2667 June 30, 2021 17:57 Inactive

Merge branch 'master' into add-v2-matviews

5fd4804

rsanheim temporarily deployed to simple-review-pr-2667 June 30, 2021 18:53 Inactive

Use RefreshMaterializedViews to refresh the new matviews

22d5c55

Merge branch 'add-v2-matviews' of https://github.com/simpledotorg/sim…

584589e

…ple-server into add-v2-matviews

rsanheim temporarily deployed to simple-review-pr-2667 June 30, 2021 19:03 Inactive

kpethtel approved these changes Jun 30, 2021

View reviewed changes

Visits depend up on States, so refresh states first

05040af

rsanheim temporarily deployed to simple-review-pr-2667 June 30, 2021 20:23 Inactive

rsanheim added 2 commits June 30, 2021 15:23

States depend up on Visits, so refresh Visits first

f1baffa

Merge branch 'add-v2-matviews' of https://github.com/simpledotorg/sim…

c7b681b

…ple-server into add-v2-matviews

rsanheim temporarily deployed to simple-review-pr-2667 June 30, 2021 20:23 Inactive

Enforce a known good time for all these specs

372b67e

rsanheim temporarily deployed to simple-review-pr-2667 June 30, 2021 20:24 Inactive

Enforce end of June for UTC, ET, and IST

50ad31b

rsanheim temporarily deployed to simple-review-pr-2667 June 30, 2021 20:35 Inactive

Freeze time to avoid intermittent failures

8439315

rsanheim temporarily deployed to simple-review-pr-2667 June 30, 2021 20:55 Inactive

rsanheim merged commit 04ece50 into master Jun 30, 2021

rsanheim deleted the add-v2-matviews branch June 30, 2021 21:00

ssrihari reviewed Jul 1, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add v2 matviews to nightly refresh #2667

Add v2 matviews to nightly refresh #2667

rsanheim commented Jun 29, 2021 •

edited

Loading

shortcut-integration bot commented Jun 29, 2021

ssrihari left a comment

ssrihari Jun 30, 2021

rsanheim Jun 30, 2021 •

edited

Loading

ssrihari Jul 1, 2021

ssrihari Jun 30, 2021

rsanheim Jun 30, 2021 •

edited

Loading

ssrihari Jul 1, 2021

ssrihari Jun 30, 2021

rsanheim Jun 30, 2021

ssrihari Jul 1, 2021

rsanheim Jul 1, 2021

ssrihari Jul 2, 2021

kpethtel Jun 30, 2021

ssrihari Jul 1, 2021

rsanheim Jul 1, 2021

ssrihari Jul 2, 2021

rsanheim Jul 4, 2021

Add v2 matviews to nightly refresh #2667

Add v2 matviews to nightly refresh #2667

Conversation

rsanheim commented Jun 29, 2021 • edited Loading

shortcut-integration bot commented Jun 29, 2021

ssrihari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsanheim Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsanheim Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsanheim commented Jun 29, 2021 •

edited

Loading

rsanheim Jun 30, 2021 •

edited

Loading

rsanheim Jun 30, 2021 •

edited

Loading