*: improve inline code comments

[Azhng]

Release note: None

[cockroach-teamcity]

This change is 

[maryliag]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei)

[andreimatei]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei and @Azhng)

---

pkg/sql/tests/server_params.go line 49 at r1 (raw file):
I see. But I still find the wide use of this knob questionable. It sounds like in production these "descriptor not found" errors are possible for new clusters, so why should so many unit tests get a blanket blue pill? How many tests would need to set this knob to keep working?

// Additionally, we don't want to completely remove the AOST clause in the
// unit test.

Any particular reason why don't we want to completely remove it? If the only reason is that removing it completely would be harder / require more code, I'd scratch the whole paragraph.

---

pkg/sql/sqlstats/persistedsqlstats/compaction_test.go line 277 at r1 (raw file):

// stats system table that are in the current aggregation window and previous
// aggregation window. Before running the SQL Stats compaction, we lower the
// row limit in the stats table so that all thw rows will be deleted by the

thw -> the

---

pkg/sql/sqlstats/persistedsqlstats/compaction_test.go line 281 at r1 (raw file):

// aggregation window. This test asserts that, since some of generated rows live
// in the current aggregation interval, those rows will not be deleted by the
// StatsCompactor.

Now I understand what's going on; thanks. Please consider adding a comment to StatsCompactor.getQargs saying that the reason the last hour is excluded is to avoid contention.

I'd consider changing the name of this test; by putting "interference" in there, you suggest that we're directly testing for "interference" - I expected a trace to be collected or something to check for the absence of contention. Instead, this test checks a straight-forward mechanism (the fact that the current interval is exempt from contention), as so I'd name it as such - consider TestSQLStatsCompactionCurrentIntervalExempt and add the comment to remind why the interval is exempt. Going deeper into nit territory :), the comment as written is a bit long and convoluted. I'd say more simply something like:

Test that stats belonging to the current aggregation interval are exempt from deletion by the stats compactor. We exempt them, at the risk of an unbounded number of stats, in order to avoid contention between the compaction job and foreground stats insertions/updates.

Btw, I don't see any checks in this test that the compactor deletes any rows (from the previous interval). So, is the part where we Generate some data that are older than the current aggregation window relevant to the test?

[Azhng]

Thanks for taking a look. I don't think I will have enough time to push this PR through before end of today so someone else prolly need to take over.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei and @Azhng)

---

pkg/sql/tests/server_params.go line 49 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I see. But I still find the wide use of this knob questionable. It sounds like in production these "descriptor not found" errors are possible for new clusters, so why should so many unit tests get a blanket blue pill? How many tests would need to set this knob to keep working?

// Additionally, we don't want to completely remove the AOST clause in the
// unit test.

Any particular reason why don't we want to completely remove it? If the only reason is that removing it completely would be harder / require more code, I'd scratch the whole paragraph.

I remember quite a few tests flaked back then and it was deflaked when this knobs are introduced. Tbh I think it's not too difficult to write a bit of more code in the knobs to clean this up.

---

pkg/sql/sqlstats/persistedsqlstats/compaction_test.go line 281 at r1 (raw file):

Btw, I don't see any checks in this test that the compactor deletes any rows (from the previous interval). So, is the part where we Generate some data that are older than the current aggregation window relevant to the test?

This is checked by writing 20 rows into the system table, then assert latter that only 10 rows remains. Since 10 out of the 20 rows written is exempted. Any rows that no longer remain are presumed to be deleted by the stats compactor.

[maryliag]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)