Partition work_orders #1392

rorymckinley · 2023-11-15T12:55:33Z

Notes for the reviewer

I tried to use as much of the existing functionality from platform as possible, to reduce the risk (given that we do add and drop tables). The SQL query that was part of PartitionTableService.remove_empty started misbehaving yesterday and I could not get to the bottom of it in a reasonable amount of time, so I decided to extract some of the logic into Elixir.

The new methods to support this were added as public methods - I felt that the improved granularity of testing outweighed the polluting of the public API :).

Also, after completing the above change, I realised that I had built a footgun into the code through a beginner's mistake in the elixr code that I added. This resulted in me extending the tests to ensure that they will hopefully catch my next stupid mistake.

Related issue

Fixes #

Review checklist

I have performed a self-review of my code
I have verified that all appropriate authorization policies have been implemented and tested
If needed, I have updated the changelog
Product has QA'd this feature

rorymckinley · 2023-11-15T12:57:52Z

lib/lightning/maintenance/partition_table_service.ex

+  end
+
+  def drop_partition(parent, partition) do
+    unless valid_chars?(parent) && valid_chars?(partition) do


String interpolation in queries makes me nervous :)- so I added some simple validation against the more obvious forms of injection (as I know that we can't bind params in DDL statements) just in case this method ends up being used someplace else.

codecov · 2023-11-15T12:59:15Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (402fd7c) 86.08% compared to head (8a0199c) 86.16%.

Files	Patch %	Lines
...b/lightning/maintenance/partition_table_service.ex	97.43%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1392      +/-   ##
==========================================
+ Coverage   86.08%   86.16%   +0.07%     
==========================================
  Files         218      220       +2     
  Lines        6452     6496      +44     
==========================================
+ Hits         5554     5597      +43     
- Misses        898      899       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rorymckinley · 2023-11-20T09:58:43Z

lib/lightning/maintenance/partition_table_service.ex

+  end
+
+  @impl Oban.Worker
+  def perform(%Oban.Job{args: %{"drop_older_than" => %{"weeks" => weeks}}})


I could not get this method to work as originally implemented in platform so I modelled the arguments on the other perform variant.

Ah interesting, this looks good to me though :)

rorymckinley · 2023-11-20T10:04:02Z

test/lightning/maintenance/partition_table_service_test.exs

+    end
+  end
+
+  defp all_relations() do


I ended up with a fair number of functions to manage the fixtures - so of which have overlaps. If the overlaps are resolved I am hoping that I could reduce the number of fixture methods.

However, I will need to extend (at least) the scenarios that cover :all use case as I partition the other tables - as such I was hoping to delay the cleanup until those scenarios have been catered for.

Thats cool, if the functions end up living in this test file thats all good imo.

rorymckinley · 2023-11-20T10:06:50Z

lib/lightning/maintenance/admin_tools.ex

@@ -0,0 +1,17 @@
+defmodule Lightning.AdminTools do
+  def generate_iso_weeks(start_date, end_date) do
+    Date.range(start_date, end_date)


When I was setting up tests for this class, I noticed that there was a small edge case that emerged when dates that are not mondays are passed in as arguments. I can't imagine that it was a big deal before, but the tweak to fix it was small, so I made an executive decision :).

test/lightning/maintenance/partition_table_service_test.exs

stuartc

Nice dude! This is awesome, I'll be wanting to test this against a backup of a real db before merging.

Are the migrations idempotent, i.e. if it borks out half way or we have to cancel it (on the unlikely case it takes too long) - will it leave behind a bunch of schemas that must be deleted before running it again? Not saying we have to cater for this right now; but I'd like to know more about the expected sad path.

stuartc · 2023-11-21T14:03:04Z

lib/lightning/maintenance/partition_table_service.ex

+  end
+
+  @impl Oban.Worker
+  def perform(%Oban.Job{args: %{"drop_older_than" => %{"weeks" => weeks}}})


Ah interesting, this looks good to me though :)

stuartc · 2023-11-21T14:07:24Z

lib/lightning/maintenance/partition_table_service.ex

+  @impl Oban.Worker
+  def perform(%Oban.Job{args: %{"drop_older_than" => %{"weeks" => weeks}}})
+      when is_integer(weeks) do
+    upper_bound = Timex.shift(DateTime.utc_now(), weeks: weeks)


Could you try using DateTime.shift instead of Timex - nothing against what you've done here - Timex was the gold standard for all things Date but the built in DateTime has come a long way since then. And we have had issues with Timex in compiled environments (like without having the entire app running, like in migrations on production); if you can achieve a passing test suite by just replacing the use of Timex then we should be good.

@stuartc I have expunged all the uses of Timex from prod code, with one exception - I can't find an equivalent to iso_week on either Date or DateTime - if you think this is going to be problematic, I can try and handroll an equivalent?

rorymckinley · 2023-11-21T16:55:28Z

Nice dude! This is awesome, I'll be wanting to test this against a backup of a real db before merging.

Are the migrations idempotent, i.e. if it borks out half way or we have to cancel it (on the unlikely case it takes too long) - will it leave behind a bunch of schemas that must be deleted before running it again? Not saying we have to cater for this right now; but I'd like to know more about the expected sad path.

Thanks for all the pointers @stuartc - I tested what I thought would the worst case by inserting a sleep in the migration just before the last up execution. I then gave it a few seconds, did a Crtrl-C followed by hitting (k)ill a few times until my screen filled with stackdumps and the thing stopped. The migration cleaned up after itself and left no half-backed artefacts. I also tried an experiment where the last instruction was broken SQl and it produced the same result - so based on this potted example, I think we should be ok. That being said: I built things based on a combination of two criteria :
(1) The tables have less than ten thousand records in them.
(2) Downtime can be tolerated.

If either of those are no longer true, I am happy to revisit the implementation.

I have made the PR changes in a new commit for ease of re-review, if you ping me before you merge I can rebase it into the original commit.

rorymckinley · 2023-12-04T09:25:26Z

This PR is being parked, pending a decision on the further partitioning of tables.

Amongst other things, this branch contains additional tests and small code tweaks (when compared to the original code) that may be useful when doing date-based partitioning.

rorymckinley · 2023-12-04T09:40:30Z

This code needs to still be tested against a database with a significant number of records in the work_orders table to gauge performance.

rorymckinley commented Nov 15, 2023

View reviewed changes

rorymckinley requested a review from stuartc November 15, 2023 12:58

rorymckinley marked this pull request as draft November 16, 2023 05:28

rorymckinley force-pushed the 1254-partition-work-orders-2 branch 6 times, most recently from 52ce270 to 3411599 Compare November 20, 2023 09:43

rorymckinley marked this pull request as ready for review November 20, 2023 09:56

rorymckinley commented Nov 20, 2023

View reviewed changes

stuartc reviewed Nov 21, 2023

View reviewed changes

test/lightning/maintenance/partition_table_service_test.exs Outdated Show resolved Hide resolved

stuartc reviewed Nov 21, 2023

View reviewed changes

rorymckinley added 2 commits November 21, 2023 18:36

Partition work_orders

7c7f32f

Review changes

8a0199c

rorymckinley force-pushed the 1254-partition-work-orders-2 branch from 3411599 to 8a0199c Compare November 21, 2023 16:44

rorymckinley requested a review from stuartc November 21, 2023 16:55

rorymckinley marked this pull request as draft December 4, 2023 09:22

rorymckinley mentioned this pull request Dec 4, 2023

Partition tables by week and setup maintenance code #1254

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partition work_orders #1392

Partition work_orders #1392

rorymckinley commented Nov 15, 2023 •

edited

rorymckinley Nov 15, 2023

codecov bot commented Nov 15, 2023 •

edited

rorymckinley Nov 20, 2023

stuartc Nov 21, 2023

rorymckinley Nov 20, 2023

stuartc Nov 21, 2023

rorymckinley Nov 20, 2023

stuartc left a comment

stuartc Nov 21, 2023

stuartc Nov 21, 2023

rorymckinley Nov 21, 2023

rorymckinley commented Nov 21, 2023

rorymckinley commented Dec 4, 2023

rorymckinley commented Dec 4, 2023

Partition work_orders #1392

Are you sure you want to change the base?

Partition work_orders #1392

Conversation

rorymckinley commented Nov 15, 2023 • edited

Notes for the reviewer

Related issue

Review checklist

Choose a reason for hiding this comment

codecov bot commented Nov 15, 2023 • edited

Codecov Report

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

stuartc left a 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

rorymckinley commented Nov 21, 2023

rorymckinley commented Dec 4, 2023

rorymckinley commented Dec 4, 2023

rorymckinley commented Nov 15, 2023 •

edited

codecov bot commented Nov 15, 2023 •

edited