alex-ninja commented on a change in pull request #1108:
URL: https://github.com/apache/cassandra/pull/1108#discussion_r671190853
##########
File path: test/unit/org/apache/cassandra/io/sstable/SSTableReaderTest.java
##########
@@ -324,24 +331,80 @@ public void testGetPositionsKeyCacheStats()
CompactionManager.instance.performMaximal(store, false);
SSTableReader sstable = store.getLiveSSTables().iterator().next();
+ // existing, non-cached key
sstable.getPosition(k(2), SSTableReader.Operator.EQ);
+ assertEquals(1, sstable.getKeyCacheRequest());
assertEquals(0, sstable.getKeyCacheHit());
- assertEquals(1, sstable.getBloomFilterTruePositiveCount());
Review comment:
I do not like having BF stats asserts in `testGetPositionsKeyCacheStats`
because they make the test scope vague (strictly speaking BF stats is
irrelevant to this test). Now we have two separate tests with clear scope (KC
stats and BF stats) and I see no benefits of having the same asserts in two
tests. In my opinion, having asserts covering the same functionality in
different tests makes working with issues harder (e.g. multiple tests fail for
the same issue, etc).
It is not a problem to recover these asserts, I just want to ensure that's
what we really want. @blerer @blambov what is your opinion on my reasoning?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
