backup: implement backup compactions after incremental backups #138347

kev-cao · 2025-01-06T20:50:08Z

This commit adds the logic for backup compaction, which is currently disabled under a boolean that is only enabled for testing. A follow-up PR will be made to add builtin performs the compaction.

Epic: none

Release note: None

blathers-crl · 2025-01-06T20:50:12Z

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2025-01-06T20:50:21Z

This change is

msbutler · 2025-01-09T22:27:58Z

@dt as a heads up, the file sink pr is landing over in #137565

blathers-crl · 2025-01-09T23:52:30Z

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

msbutler

Looks good! left mostly nits.

msbutler · 2025-01-10T14:46:59Z

pkg/backup/backup_job.go

@@ -999,6 +1000,10 @@ func (b *backupResumer) Resume(ctx context.Context, execCtx interface{}) error {
 		logutil.LogJobCompletion(ctx, b.getTelemetryEventType(), b.job.ID(), true, nil, res.Rows)
 	}

+	if err := maybeCompactIncrementals(ctx, p, origDetails, b.job.ID()); err != nil {


i'd leave a todo that in the future, this function will merely write a job record to run a backup compaction job.

msbutler · 2025-01-10T15:22:58Z

pkg/backup/backup_compaction.go

+
+var backupCompactionThreshold = settings.RegisterIntSetting(
+	settings.ApplicationLevel,
+	"bulkio.backup.compaction.unsafe.threshold",


i think you can remove the unsafe part of the cluster setting name.

msbutler · 2025-01-10T15:26:10Z

pkg/backup/backup_compaction.go

+	jobID jobspb.JobID,
+) error {
+	threshold := backupCompactionThreshold.Get(&execCtx.ExecCfg().Settings.SV)
+	if threshold == 0 || !lastIncDetails.Destination.Exists || lastIncDetails.RevisionHistory {


i think we should only run compaction backups on scheduled backups. details.ScheduleID would provide that info.

pkg/backup/backup_compaction.go

msbutler · 2025-01-10T16:43:33Z

pkg/backup/backup_compaction.go

+	if err != nil {
+		return err
+	}
+	if prefix != nil {


lets assume there's a prefix and error if it is nil? Then, we can call these prefixlessStartKey and prefixlessEndKey`

msbutler · 2025-01-10T16:56:45Z

pkg/backup/backup_compaction.go

+			return err
+		}
+		fullKey := key
+		scratch = append(scratch[:len(prefix)], key.Key...)


ok interesting. i hadn't realized this until now, but it is a bit silly that we prepend the prefix only to strip it in WriteKey. So, suppose we remove this wasted work, how does that complicate things?

it would be harder to construct file spans which must contain full keys

we can't validate that a key is within key span

I'm not sure if EnsureSafeSplitKey works on prefixles keys.

Given this, i'm fine having this little bit of wasted work. It will not be the bottle neck of backup compaction anyway.

Yeah when I was writing the SSTSinkKeyWriter, I intentionally used the full key because of those complications you listed above. And looking at EnsureSafeSplitKey, I don't think it does.

it would be harder to construct file spans which must contain full keys

Are there any of these? We mentioned we weren't doing the no-elision mode.

Regardless of the elision mode, the file entry spans must be full keys. You've already coded it this way, as we do the size based flush in writeKey before eliding the key.

i think we could add a little docstring here that motivates this extra computation.

msbutler · 2025-01-10T17:00:51Z

pkg/backup/backup_compaction.go

+		if err != nil {
+			return err
+		}
+		fullKey := key


could we operate directly on key.key instead of creating a new local var fullKey?

Oh yup, for some reason I thought this would clobber something but nope it certainly doesn't

pkg/backup/backup_compaction.go

msbutler · 2025-01-10T17:16:40Z

pkg/backup/backup_compaction.go

+}
+
+// concludeBackupCompaction completes the backup compaction process after the backup has been
+// completed by writing the manifest and associated metadata to the backup destination.


maybe once this is jobified, this could become a helper shared by regular and compacted backups

kev-cao · 2025-01-14T22:09:42Z

pkg/backup/backup_compaction_test.go

+				}), ", "),
+			)
+
+			t.Run("inserts", func(t *testing.T) {


nit: Do we have a preference over nested t.Run vs flattening it out and doing t.Run(fmt.Sprintf("%s/inserts", tc.name))? Downside to the latter would be having to adding the Sprintf to each of the t.Run.

I don't think we're adding that much test coverage with the following test cases: run backup compaction on an insert only workload, then on update only workload, then with deletes etc. etc. We could probably achieve the same coverage by running one backup compaction on a chain with a single insert, a single insert + update, and a since insert + delete. This could encompass a "basic" subtest. Additional subtests could include exercising the introduced spans logic (create a table, create an index), and dropping a table and index. Perhaps each case should also run a compaction on a compacted backup.

The question we want to answer is: what test cases would provide coverage over the nooks and crannies this diff adds?

happy to chat over a call about other test cases to add.

last thought: once we add the bare bones tests, we could consider metamorphically running a backup compaction on a bunch of different existing backup tests to add coverage around kms, encryption and other things. we should consider this in a follow up pr, perhaps after creating a roachtest.

kev-cao · 2025-01-14T22:13:07Z

pkg/backup/backup_compaction.go

+
+// compactIntroducedSpans takes a compacted backup manifest and the full chain of backups it belongs to and
+// computes the introduced spans for the compacted backup.
+func compactIntroducedSpans(


We could add unit tests here, although it's covered in the TestBackupCompaction through the use of creating tables/indexes during incrementals...

msbutler · 2025-01-15T14:19:50Z

pkg/backup/backup_compaction.go

 	"github.com/cockroachdb/cockroach/pkg/util/uuid"
 	"github.com/cockroachdb/errors"
 	"github.com/gogo/protobuf/types"
 )

 var backupCompactionThreshold = settings.RegisterIntSetting(
 	settings.ApplicationLevel,
-	"bulkio.backup.compaction.threshold",
+	"backup.compaction.threshold",
 	"backup chain length threshold at which backup compaction is triggered "+
 		"(0 to disable compactions)",
 	0,
 	settings.WithUnsafe,


i'd remove this unsafe thing

Should the name at least be prefixed with unsafe or experimental?

Apologies for a late drive-by here but:

I’d suggest that we actually don’t want a setting, or the integration into backup itself the setting controls, yet: I’d make a standalone SQL command (builtin) that we can run against an explicit backup collection and path+times to explicitly compact a specific set of backups, or maybe zeros let it pick one, but still explicitly triggered and blocks until done. I think this is easier to test in isolation — we can setup specific conditions and try different compactions on them, and then a follow-up change can integrate calling that into a schedule.

I can see the benefit of having a separate builtin for compactions, but won't that result in a decent amount of work that comes from adding a new syntax, just to ultimately get removed entirely?

At least, the end goal Michael and I had pictured was that when a scheduled backup job completes, it would potentially kick off a compaction job. I'd see the benefit of adding the builtin if a compaction job could be triggered manually (i.e. a separate schedule that runs a compaction job), but not so much with the current goal in mind.

builtins functions, in contrast to new syntax, are pretty minimal work -- no sql.y, no parser/tree/etc, none of that; just an entry in a map and then reserving an OID for it.

I think it makes sense to start this as a standalone function both because this makes it easier to test -- by which I mean unit test and roachtest -- and because I think we might want that function at our disposal in general. You could easily imagine that in the short term this isn't on by default, but we maybe elect to call it one-off if we're faced with a really troublesome restore or something.

Since I think we want it as a manual tool anyway it doesn't feel like wasted work to ship it as a manual tool initially, and then integrate automatic calls to it into scheduled backups later, after we've done all the testing and development on the tool version?

I was thinking crdb_internal.backup_compaction(collection, backup, starttime, endtime)

what would the backup arg provide?

You need to identify which backup (chain) in the collection contains the inc layers you want to compact

oh right. duh.

e.g. crdb_internal.backup_compaction('s3://my-cool-backups', '2024/10/22-000000.00', startTS, endTs)

msbutler · 2025-01-15T14:35:01Z

pkg/backup/backup_compaction.go

+	// TODO (kev-cao): Will need to update the SSTSinkKeyWriter to support
+	// range keys.
+	if len(tenantSpans) != 0 || len(tenantInfos) != 0 {
+		err = errors.New("Backup compactions does not yet support range keys.")


uber nit: error messages typically do not begin capitalized or end in a period. see our style guidelines https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/1676804870/Error+concepts+and+handling+in+CockroachDB

msbutler · 2025-01-15T14:43:50Z

pkg/backup/backup_compaction.go

+	encryption *jobspb.BackupEncryptionOptions,
+	kmsEnv cloud.KMSEnv,
+	layerToIterFactory backupinfo.LayerToBackupManifestFileIterFactory,
+) (


in general, we refrain from using named return vars because the return line on error looks ambiguous within the function. we really only use it to deal with defer logic.

Also, i don't think returning dest is necessary. Looks like only dest.defaultURI is ever used by the caller which also lives in details.URI

Oh sure, will update, good to keep in mind

msbutler · 2025-01-15T14:49:25Z

pkg/backup/backup_compaction.go

+
+// getIterFactoriesForCompaction returns the layer to iterator factories for both the entire
+// backup chain and the chain of backups to be compacted.
+func getIterFactoriesForCompaction(


i think this function could be inlined, and then move the creation of compactedIters closer to where it is used. Also these are iteratory factories not iterators.

msbutler · 2025-01-15T14:51:41Z

pkg/backup/backup_compaction.go

+	}
+
+	progCh := make(chan execinfrapb.RemoteProducerMetadata_BulkProcessorProgress)
+	processProg := func(


i don't think this needs to be a closure.

Do you mean it doesn't need to be in a named closure? It still would end up being in a closure when it gets appended to the wait group tasks.

nah, i'm saying this func could be a first class function, instead of a closure, as it doesn't close over any local vars.

msbutler · 2025-01-15T15:10:43Z

pkg/backup/backup_compaction.go

+	if err != nil {
+		return err
+	}
+	catalogDescs := make([]catalog.Descriptor, 0, len(backupManifest.Descriptors))


doesn't look like this is used for anything

msbutler · 2025-01-15T15:18:58Z

pkg/backup/backup_compaction_test.go

+
+	compactionThreshold := 3
+	settings := cluster.MakeTestingClusterSettings()
+	backupCompactionThreshold.Override(ctx, &settings.SV, int64(compactionThreshold))


nit: you could always set the setting via sql after the cluster initializes.

Yeah will do — couldn't do it because of the unsafe thing, but if we remove then it'll work

msbutler · 2025-01-15T15:26:20Z

pkg/backup/backup_compaction_test.go

+			},
+			[]string{
+				fmt.Sprintf(
+					"nodelocal://1/backup?COCKROACH_LOCALITY=%s",


i dont think the locality aware backups case needs to run through every single combo you provide below. We merely want to test that uri resolution works for backup compaction. So, I don't think this high level dd framework is necessary-- rather, this would be much easier to read if the locality aware test was its own stand alone test.

msbutler · 2025-01-15T15:40:25Z

pkg/backup/backup_compaction_test.go

+				}), ", "),
+			)
+
+			t.Run("inserts", func(t *testing.T) {


I don't think we're adding that much test coverage with the following test cases: run backup compaction on an insert only workload, then on update only workload, then with deletes etc. etc. We could probably achieve the same coverage by running one backup compaction on a chain with a single insert, a single insert + update, and a since insert + delete. This could encompass a "basic" subtest. Additional subtests could include exercising the introduced spans logic (create a table, create an index), and dropping a table and index. Perhaps each case should also run a compaction on a compacted backup.

The question we want to answer is: what test cases would provide coverage over the nooks and crannies this diff adds?

happy to chat over a call about other test cases to add.

last thought: once we add the bare bones tests, we could consider metamorphically running a backup compaction on a bunch of different existing backup tests to add coverage around kms, encryption and other things. we should consider this in a follow up pr, perhaps after creating a roachtest.

msbutler · 2025-01-15T15:41:10Z

pkg/backup/backup_compaction_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestBackupCompactionTriggered(t *testing.T) {


this test is merely checking that a backup compaction ran? could we add this single check in the test below?

msbutler

Prod code looks good! let me know if you want to chat about testing strategy. Also, how long do these tests take to run? I'd think that reducing the number sql queries would also reduce the test runtime nicely.

msbutler · 2025-01-23T14:49:17Z

pkg/backup/backup_compaction.go

+	if len(manifests) == 0 {
+		return jobspb.BackupDetails{}, errors.New("no backup manifests to compact")
+	}
+	compactedDetails, err := updateBackupDetails(


it's unclear to me exactly why this is necessary, but i'm fine keeping this for now as we may need to refactor this once this is triggered by the built in.

msbutler · 2025-01-23T15:01:26Z

pkg/backup/backup_compaction_test.go

+			t, db, "'nodelocal://1/backup'",
+			insertRows(20, "foo"), []string{}, false,
+		)
+		runQueriesAndBackup(


why are two incrementals created before creating the table? ditto with the create index test case.

I wanted to have multiple incrementals being compacted instead of just the single one — although maybe I should have those incrementals also add a table/index

msbutler · 2025-01-23T15:11:50Z

pkg/backup/backup_compaction_test.go

+		t, singleNode, tempDir, InitManualReplication, base.TestClusterArgs{},
+	)
+	defer cleanupDB()
+	insertRows := func(nRows int, table string) []string {


it's still not clear to me why this test requires generating a random number of deletes, inserts and updates. I also think you could reduce the number of lines in each test case by calling the sql directly, rather than using the runQueriesAndBackup helper. I think removing runQueriesAndBackup would also make these test cases easier to read. The validateCompactedBackup func is certainly useful though.

Happy to chat more about this.

Well the number of deletes/inserts/updates are constant — do you mean the rows that are being updated/deleted? I'm choosing to make them random so that we have a mix of rows that were updated and then deleted, or just deleted, etc. (Although I don't have to randomly generate the values in the row)

One of the benefits of having the queries run in a separate function is that it makes it easier to toggle compaction on and off — otherwise we'd have to explicitly turn them on and back off manually before running a backup.

msbutler · 2025-01-23T15:12:51Z

pkg/backup/backup_compaction_test.go

+		}
+		return insertQueries
+	}
+	runQueriesAndBackup(t, db, collectionURIs, createInsertQueries(10), []string{}, false)


why is it necessary to run all these insert and update queries to test that we handle locality aware backups properly?

Updated the code so that it's one insert statement instead of several, but to answer your question, I wanted to have some deltas in each backup to ensure that compaction was doing something meaningful so that it could be validated.

msbutler · 2025-01-23T15:13:30Z

pkg/backup/backup_compaction_test.go

+	// iterator, add tests for dropped tables/indexes.
+}
+
+func TestBackupCompactionMultiRegion(t *testing.T) {


nit: s/MultiRegion/LocalityAware/r

msbutler

Prod code looks good! let me know if you want to chat about testing strategy. Also, how long do these tests take to run? I'd think that reducing the number sql queries would also reduce the test runtime nicely.

kev-cao · 2025-01-23T16:37:11Z

@msbutler TestBackupCompaction takes about 4 seconds, and TestBackupCompactionLocalityAware takes about 2 seconds.

msbutler · 2025-01-23T21:54:49Z

pkg/backup/backup_compaction_test.go

+// doIncrementalBackup performs an incremental backup, and if compact is set to true, performs a
+// compaction afterward.
+// TODO (kev-cao): Remove once doCompaction bool is removed and builtin for compaction is made.
+func doIncrementalBackup(


this feels much cleaner. nice!

msbutler · 2025-01-23T21:57:26Z

pkg/backup/backup_compaction_test.go

+		}()
+		db.Exec(t, fullBackupStmt)
+		for i, tbl := range tables {
+			db.Exec(t, fmt.Sprintf("CREATE TABLE %s (a INT, b INT)", tbl))


nit: i think this test should mimic the structure of the create index test case as it exercises introducing spans across a couple layers. also, maybe drop one of the tables as well?

If by structure you mean checking if the table exists, I think it does cover it since validateCompactedBackup does check the contents of each table specified.

Doesn't dropping the tables lay a range key tombstone? I thought it did, which is why this doesn't test it just yet.

by structure i mean:

db.Exec(t, fullBackupStmt) db.Exec(t, "CREATE TABLE foo") // insert into foo doIncrementalBackup(t, db, "'nodelocal://1/backup'", false) db.Exec(t, "CREATE TABLE BAZ") // insert into baz doIncrementalBackup(t, db, "'nodelocal://1/backup'", true)

Dropping tables does write a tombstone, but if you're backup compaction code is correct, we should not attempt to compact the dropped table, thus avoiding the need to handle the tombstone!

The compaction iterator would ignore the tombstone range key, so the deletion of the table is not part of the compacted backup. So the original incrementals have the table dropped, but restoring from the compacted backup would show the table as never been dropped to begin with?

^Assuming the drop of the table occurs in a separate incremental from the table being created.

Recall that a backup does not back up dropped tables. So imagine the following timeline:

t0: create table foo
t1: full backup of table foo
t1: inc backup of table foo
t2: drop table foo
t3: inc backup of table foo (will not back up table foo key space, which will contain a range key)

So, if we compact the two incrementals, i do not expect table foo to be in the target compacted backup, as we grab the table targets from the second incremental:
https://github.com/kev-cao/cockroach/blob/backup/compaction/pkg/backup/backup_compaction.go#L185

Since we shouldn't be putting foo in our list of tables to create a compacted backup of, i do not expect the compaction process to read any of table foo's key space.

I was under the impression that a full backup won't back up a dropped table, but because the previous incremental had the table in the backup, the range key had to be recorded in the second incremental.

But I didn't account for the fact that the table had to be written in the manifest's descriptors as well. So when it comes to recording the dropping of a table, the tombstone range key never has to be written? The table just has to be excluded from the manifest's list of descriptors?

Does this mean that as far as backup is concerned, the only range key that needs to be kept in mind is dropped indexes (and dropped tenants)?

I remember there was discussion about import rollbacks creating tombstones as well. Since those import rollbacks could include both tables and indexes, does backup only record the dropping of the indexes?

I was under the impression that a full backup won't back up a dropped table, but because the previous incremental had the table in the backup, the range key had to be recorded in the second incremental.

ah, so this is subtle. non revision history backups, the only ones backup compaction will ever have to deal with, will not back up tables dropped as of their end time. But, revision history backups will need to back up a table that may have been dropped before its end time! See the logic here:
https://github.com/kev-cao/cockroach/blob/backup/compaction/pkg/backup/backup_job.go#L1626

if you follow this rabbit hole, you'll notice that the Descriptors field in the manifest contains online tables as of the end time, while revision history backups also write the DescriptorChanges field.

So, since backup compaction only ever deals with non revision history backups, we should not expect to read data from a dropped table.

Does this mean that as far as backup is concerned, the only range key that needs to be kept in mind is dropped indexes (and dropped tenants)?

Non revision history backups also skip backing up dropped indexes (which are not public).
https://github.com/kev-cao/cockroach/blob/backup/compaction/pkg/backup/backup_job.go#L1184
(Or, at least they are supposed to. There may actually be a small bug here where ForEachPublicIndex doesnt properly filter out non public indexes that have yet to be removed from the index list.)

Further, restore will also filter out any dropped indexes from its targets it needs to restore:
https://github.com/kev-cao/cockroach/blob/backup/compaction/pkg/backup/restore_job.go#L838

I remember there was discussion about import rollbacks creating tombstones as well. Since those import rollbacks could include both tables and indexes, does backup only record the dropping of the indexes?

Jeff is refactoring import rollbacks to use regular old point tombstones instead.

All this is to say, the compaction should never see a range key unless it compacts a backup of a tenant.

This commit adds the logic for backup compaction, which is currently disabled under a boolean that is only enabled for testing. A follow-up PR will be made to add builtin performs the compaction. Epic: none Release note: None

kev-cao force-pushed the backup/compaction branch 3 times, most recently from c0acbc6 to ebfb92c Compare January 9, 2025 22:20

kev-cao requested review from dt and msbutler January 9, 2025 22:21

kev-cao force-pushed the backup/compaction branch from ebfb92c to a9574ee Compare January 9, 2025 23:52

msbutler reviewed Jan 10, 2025

View reviewed changes

kev-cao force-pushed the backup/compaction branch 4 times, most recently from 9a52530 to cfd8fd1 Compare January 14, 2025 22:07

kev-cao marked this pull request as ready for review January 14, 2025 22:08

kev-cao requested a review from a team as a code owner January 14, 2025 22:08

kev-cao commented Jan 14, 2025

View reviewed changes

kev-cao force-pushed the backup/compaction branch from cfd8fd1 to 28b481b Compare January 14, 2025 22:10

kev-cao commented Jan 14, 2025

View reviewed changes

kev-cao added the do-not-merge bors won't merge a PR with this label. label Jan 14, 2025

msbutler reviewed Jan 15, 2025

View reviewed changes

kev-cao force-pushed the backup/compaction branch 3 times, most recently from d745a9c to 9d398a5 Compare January 22, 2025 20:36

kev-cao removed the do-not-merge bors won't merge a PR with this label. label Jan 22, 2025

msbutler reviewed Jan 23, 2025

View reviewed changes

kev-cao force-pushed the backup/compaction branch 2 times, most recently from 5ddec0b to 1a8ee90 Compare January 23, 2025 16:15

kev-cao force-pushed the backup/compaction branch 2 times, most recently from bdb2691 to 424b50e Compare January 23, 2025 16:36

kev-cao force-pushed the backup/compaction branch from 424b50e to b6fca04 Compare January 23, 2025 20:30

msbutler approved these changes Jan 23, 2025

View reviewed changes

kev-cao force-pushed the backup/compaction branch 2 times, most recently from 11645ff to b4ff943 Compare January 24, 2025 23:13

kev-cao force-pushed the backup/compaction branch from b4ff943 to 2711cef Compare January 25, 2025 02:58

backup: implement backup compactions after incremental backups #138347

Are you sure you want to change the base?

backup: implement backup compactions after incremental backups #138347

Conversation

kev-cao commented Jan 6, 2025 • edited Loading

blathers-crl bot commented Jan 6, 2025

cockroach-teamcity commented Jan 6, 2025

msbutler commented Jan 9, 2025

blathers-crl bot commented Jan 9, 2025

msbutler 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

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

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

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

msbutler 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

msbutler Jan 23, 2025 • 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

msbutler left a comment

Choose a reason for hiding this comment

kev-cao commented Jan 23, 2025

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

kev-cao Jan 23, 2025 • 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

kev-cao commented Jan 6, 2025 •

edited

Loading

msbutler Jan 23, 2025 •

edited

Loading

kev-cao Jan 23, 2025 •

edited

Loading