ref(grouping): Clarify Seer grouping deletion code

[armenzg]

This will make my next pull request easier to review.

Ref inc-1236

[armenzg]

We're going to use a constant instead of this.

[armenzg]

The option is set to 100, thus, let's go with that.

[armenzg]

I'm a bit shocked we don't retry. Any thoughts as to why we shouldn't?

[markstory]

You could add a retry handler. Make sure you allow list one or more exception types though. Without that retries don't happen unless a TimeoutError is raised.

[armenzg]

Do you have some good examples I can look at?

[codecov]

Codecov Report

Attention: Patch coverage is 61.11111% with 7 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
.../sentry/tasks/test_delete_seer_grouping_records.py	57.14%	3 Missing ⚠️
src/sentry/tasks/delete_seer_grouping_records.py	50.00%	2 Missing ⚠️
src/sentry/seer/similarity/grouping_records.py	66.66%	1 Missing ⚠️
...ts/sentry/seer/similarity/test_grouping_records.py	75.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #95152       +/-   ##
===========================================
- Coverage   87.89%   74.72%   -13.17%     
===========================================
  Files       10458    10457        -1     
  Lines      604829   604842       +13     
  Branches    23607    23607               
===========================================
- Hits       531616   451982    -79634     
- Misses      72852   152499    +79647     
  Partials      361      361               

[markstory]

Looks good to me.

[markstory]

You could add a retry handler. Make sure you allow list one or more exception types though. Without that retries don't happen unless a TimeoutError is raised.

[cursor]

Bug: Test Fails When Batch Size Changes

The test_delete_seer_grouping_records_by_hash_batches assertion hardcodes the expected task end_index as 100. This value should instead use the dynamically calculated batch_size variable, which is derived from the embeddings-grouping.seer.delete-record-batch-size option. If this option's value differs from 100, the test will incorrectly fail.

tests/sentry/tasks/test_delete_seer_grouping_records.py#L28-L36

sentry/tests/sentry/tasks/test_delete_seer_grouping_records.py

Lines 28 to 36 in b95cc57

	"""
	batch_size = options.get("embeddings-grouping.seer.delete-record-batch-size") or 100
	mock_call_seer_to_delete_these_hashes.return_value = True
	project_id, hashes = 1, [str(i) for i in range(batch_size + 1)]
	# We call it as a function and will schedule a task for the extra hash
	delete_seer_grouping_records_by_hash(project_id, hashes, 0)
	assert mock_delete_seer_grouping_records_by_hash_apply_async.call_args[1] == {
	"args": [project_id, hashes, 100]
	}

Fix in Cursor • Fix in Web

---

Was this report helpful? Give feedback by reacting with 👍 or 👎