CASSANDRA-14888 Several mbeans are not unregistered when dropping a keyspace and table

[stillalex]

No description provided.

[maedhroz]

@stillalex There are two kinds of metrics for MVs. The first represents the conditionally registered metrics in TableMetrics, for example viewReadTime. Then you have ViewWriteMetrics which is created on the StorageProxy and spans tables. (I think I mentioned the latter a bit too carelessly in one of my earlier comments on the Jira. Apologies.)

In any case, all I think we need to do is drop this test at the end of TableMetricsTest (maybe renaming to testViewMetricsCleanupOnDrop())and we're golden. That will ensure that even MVs-specific metrics in TableMetrics are removed. Does that make sense?

[stillalex]

I'll move the test over, sure. the trouble is I'm trying to verify (manually) that those metrics are actually created, otherwise that part of the code is not covered, so the test is not actually testing anything :)

[maedhroz]

An MV actually has fewer metrics than a normal table, due to ViewLockAcquireTime and ViewReadTime being present only for the latter. This test is still valuable, as it makes sure removal still works when those aren't present (i.e. the conditional logic in releaseMetric()).

Either way, I don't think we need to worry about ViewWriteMetrics.

[maedhroz]

If you want to verify here that ViewLockAcquireTime and ViewReadTime are created only the base table and not an MV, you could filter the names on each of them, etc.

[maedhroz]

Suggested change

	for(ReleasableMetric metric : allMetrics)
	for (ReleasableMetric metric : allMetrics)

[maedhroz]

Suggested change

	for(ReleasableMetric entry : all)
	for (ReleasableMetric entry : all)

[stillalex]

same for KeyspaceMetrics

[stillalex]

@maedhroz updated the PR based on your suggestions, let me know if you want to dig deeper into the MV tests, or if this is sufficient.

[maedhroz]

@stillalex LGTM

The only other thing I might do w/ the tests is, if any metrics remain after drop, print their names. It might save posterity some time hunting down the offender(s) ;)

Thanks again for the patch!

[stillalex]

@maedhroz very good suggestion! I changed the tests to include the metrics in the error message.

[maedhroz]

@stillalex I think we're probably good to commit this, although squashing first might make it easier for the committer ;)