You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2021/07/15 18:18:38 UTC
[GitHub] [cassandra] alex-ninja opened a new pull request #1108: CASSANDRA-12922. Bloom filter miss counts are not measured correctly.
alex-ninja opened a new pull request #1108:
URL: https://github.com/apache/cassandra/pull/1108
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] alex-ninja commented on a change in pull request #1108: CASSANDRA-12922. Bloom filter miss counts are not measured correctly.
Posted by GitBox <gi...@apache.org>.
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] alex-ninja commented on pull request #1108: CASSANDRA-12922. Bloom filter miss counts are not measured correctly.
Posted by GitBox <gi...@apache.org>.
alex-ninja commented on pull request #1108:
URL: https://github.com/apache/cassandra/pull/1108#issuecomment-882544286
Merged.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] alex-ninja closed pull request #1108: CASSANDRA-12922. Bloom filter miss counts are not measured correctly.
Posted by GitBox <gi...@apache.org>.
alex-ninja closed pull request #1108:
URL: https://github.com/apache/cassandra/pull/1108
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] alex-ninja commented on pull request #1108: CASSANDRA-12922. Bloom filter miss counts are not measured correctly.
Posted by GitBox <gi...@apache.org>.
alex-ninja commented on pull request #1108:
URL: https://github.com/apache/cassandra/pull/1108#issuecomment-882544286
Merged.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] alex-ninja commented on pull request #1108: CASSANDRA-12922. Bloom filter miss counts are not measured correctly.
Posted by GitBox <gi...@apache.org>.
alex-ninja commented on pull request #1108:
URL: https://github.com/apache/cassandra/pull/1108#issuecomment-882544286
Merged.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] alex-ninja closed pull request #1108: CASSANDRA-12922. Bloom filter miss counts are not measured correctly.
Posted by GitBox <gi...@apache.org>.
alex-ninja closed pull request #1108:
URL: https://github.com/apache/cassandra/pull/1108
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] blerer commented on a change in pull request #1108: CASSANDRA-12922. Bloom filter miss counts are not measured correctly.
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #1108:
URL: https://github.com/apache/cassandra/pull/1108#discussion_r671134326
##########
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:
Even if there is a more in depth check of `BloomFilterTruePositiveCount` we still could keep that kind of assertions in this test.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] blerer commented on a change in pull request #1108: CASSANDRA-12922. Bloom filter miss counts are not measured correctly.
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #1108:
URL: https://github.com/apache/cassandra/pull/1108#discussion_r671243879
##########
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:
Fine with me.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] alex-ninja closed pull request #1108: CASSANDRA-12922. Bloom filter miss counts are not measured correctly.
Posted by GitBox <gi...@apache.org>.
alex-ninja closed pull request #1108:
URL: https://github.com/apache/cassandra/pull/1108
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org