GEODE-9999: Update redis API hits/misses stats for string-based commands correctly #3
Conversation
…nds correctly - Native redis only updates the hits/misses statistics for operations which retrieve data. Currently, our implementation updates these stats for operations which also mutate data. This change aligns our statistics with native redis.
jdeppe-pivotal
force-pushed
the
fake-interview-pr-2
branch
from
11087ea to
ad75cf5
Compare
| @@ -474,27 +474,27 @@ public void NetworkKiloBytesReadOverLastSecond_shouldReturnCorrectData() { | |||
| @Test | |||
| public void clientsStat_withConnectAndClose_isCorrect() { | |||
|
|
|||
| jedis = new Jedis("localhost", server.getPort(), TIMEOUT); | |||
| jedis.ping(); | |||
| Jedis jedis2 = new Jedis("localhost", server.getPort(), TIMEOUT); | |||
There was a problem hiding this comment.
@bob What is this change for? Seems unrelated to the PR.
There was a problem hiding this comment.
While testing, I noticed that there was a problem with the stat for
connected clients. Occasionally this test and the next one would fail. My fix seems to have addressed the issue.
There was a problem hiding this comment.
@bob I doubt you ran it enough times since it's clearly not the correct fix. The real problem is that when the connection is closed the stat which counts the connections is updated asynchronously. This means that if you read the stat immediately after a close it may not be correct. You need to wait for the stat to 'settle' before using it.
There was a problem hiding this comment.
I'm not sure I follow you. I've run this fix a bunch of times on my laptop and it always passes.
There was a problem hiding this comment.
@bob We've spoken about multi threading before and the issues with concurrency. When 2 threads do something at the same time (or one thread does something in the background as is the case here) you cannot assume to always know immediately when that work is done.
There was a problem hiding this comment.
Maybe point me at a failing test and prove your point for once, instead of nitpicking and making condescending comments. No wonder everything around here takes 3x as long as it should if we have people like you gumming up the works.
| if (redisData == NULL_REDIS_STRING) { | ||
| redisStats.incKeyspaceMisses(); | ||
| if (affectsRedisStatistics) |
There was a problem hiding this comment.
@bob Did you run spotless before submitting this PR? Please add a block for these single statements
There was a problem hiding this comment.
I did run spotless. This code is not incorrect and I've seen this being used elsewhere in our codebase. Why are you always nitpicking my code?
There was a problem hiding this comment.
@bob Just because Spotless didn't pick it up doesn't mean it's not part of our coding style. It makes for consistency and reduces the possibility of introducing future errors which I've seen happen when someone merges code that looks like this.
There was a problem hiding this comment.
Why are you always nitpicking my code?
| } | ||
|
|
||
| @Override | ||
| public ByteArrayWrapper getset(ByteArrayWrapper key, ByteArrayWrapper value) { | ||
| return stripedExecute(key, | ||
| () -> getRedisString(key).getset(getRegion(), key, value)); | ||
| () -> getRedisString(key, false).getset(getRegion(), key, value)); |
There was a problem hiding this comment.
@bob This is wrong - it should be true here.
There was a problem hiding this comment.
The story says that we should only update stats for those commands that retrieve data.
There was a problem hiding this comment.
@bob Well, this command does both. It gets a value and then sets a new value so we do need to update the stat. Did you even check how native redis handles this before assuming you knew what to do here?
|
|
||
| assertThat(redisStats.getConnectionsReceived()).isEqualTo(1); | ||
| assertThat(redisStats.getConnectedClients()).isEqualTo(0); |
There was a problem hiding this comment.
@bob This assertion should not be changed.
There was a problem hiding this comment.
@bob If you had read the name of the test method you'd see that it's supposed to be asserting on the connections received and not on the number of connected clients.
There was a problem hiding this comment.
OK - I guess it's not very clear that that was the intention.
which retrieve data. Currently, our implementation updates these stats
for operations which also mutate data. This change aligns our
statistics with native redis.
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically
develop)?Is your initial contribution a single, squashed commit?
Does
gradlew buildrun cleanly?Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to [email protected].