You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by ifesdjeen <gi...@git.apache.org> on 2018/10/05 15:19:55 UTC
[GitHub] cassandra pull request #278: Avoid running query to self through messaging s...
GitHub user ifesdjeen opened a pull request:
https://github.com/apache/cassandra/pull/278
Avoid running query to self through messaging service
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ifesdjeen/cassandra avoid-querying-self-through-ms
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cassandra/pull/278.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #278
----
commit 007bf2611f54d24251c39fe7733394bfb67e0dd8
Author: Alex Petrov <al...@...>
Date: 2018-10-05T14:50:52Z
Avoid running query to self through messaging service
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #278: Avoid running query to self through messaging s...
Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/278#discussion_r223398346
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1078,7 +1078,7 @@ public void sendOneWay(MessageOut message, int id, InetAddressAndPort to)
logger.trace("{} sending {} to {}@{}", FBUtilities.getBroadcastAddressAndPort(), message.verb, id, to);
if (to.equals(FBUtilities.getBroadcastAddressAndPort()))
- logger.trace("Message-to-self {} going over MessagingService", message);
+ logger.warn("Message-to-self {} going over MessagingService", message);
--- End diff --
My last thought is that as a user I can't do much with this information. It's mostly harmless, but I also can't fix it other than by filing a bug report.
I think this is the biggest issue with this change. It will cause people worry when nothing is wrong. I think this should be debug not warn because no action is required on part of a user (as opposed to a developer).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #278: Avoid running query to self through messaging s...
Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/278#discussion_r223396208
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1078,7 +1078,7 @@ public void sendOneWay(MessageOut message, int id, InetAddressAndPort to)
logger.trace("{} sending {} to {}@{}", FBUtilities.getBroadcastAddressAndPort(), message.verb, id, to);
if (to.equals(FBUtilities.getBroadcastAddressAndPort()))
- logger.trace("Message-to-self {} going over MessagingService", message);
+ logger.warn("Message-to-self {} going over MessagingService", message);
--- End diff --
Use NoSpamLogger and maybe for the key incorporate the stack trace so we rate limit every call site separately?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #278: Avoid running query to self through messaging s...
Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/278#discussion_r223397323
--- Diff: src/java/org/apache/cassandra/service/reads/repair/AbstractReadRepair.java ---
@@ -102,12 +104,24 @@ void sendReadCommand(Replica to, ReadCallback readCallback, boolean speculative)
else type = to.isFull() ? "full" : "transient";
Tracing.trace("Enqueuing {} data read to {}", type, to);
}
- MessageOut<ReadCommand> message = command.createMessage();
- // if enabled, request additional info about repaired data from any full replicas
- if (command.isTrackingRepairedStatus() && to.isFull())
- message = message.withParameter(ParameterType.TRACK_REPAIRED_DATA, MessagingService.ONE_BYTE);
- MessagingService.instance().sendRRWithFailure(message, to.endpoint(), readCallback);
+ if (to.isSelf())
+ {
+ try (ReadExecutionController executionController = command.executionController();
--- End diff --
Just asking the question, should this occur in this thread or the read stage?
Is there a possibility of this operation succeeding if the local portion times out? If there is maybe it shouldn't be done synchronously in the request stage?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra issue #278: Avoid running query to self through messaging service
Posted by ifesdjeen <gi...@git.apache.org>.
Github user ifesdjeen commented on the issue:
https://github.com/apache/cassandra/pull/278
Thank you for the review!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #278: Avoid running query to self through messaging s...
Posted by ifesdjeen <gi...@git.apache.org>.
Github user ifesdjeen closed the pull request at:
https://github.com/apache/cassandra/pull/278
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #278: Avoid running query to self through messaging s...
Posted by ifesdjeen <gi...@git.apache.org>.
Github user ifesdjeen commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/278#discussion_r223421365
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1078,7 +1078,7 @@ public void sendOneWay(MessageOut message, int id, InetAddressAndPort to)
logger.trace("{} sending {} to {}@{}", FBUtilities.getBroadcastAddressAndPort(), message.verb, id, to);
if (to.equals(FBUtilities.getBroadcastAddressAndPort()))
- logger.trace("Message-to-self {} going over MessagingService", message);
+ logger.warn("Message-to-self {} going over MessagingService", message);
--- End diff --
Right, we can do debug here. I wanted to first throw in this case, but then thought that it's more useful to find all the cases where we still do that and eliminate those, since failing in that case brings more or less nothing.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #278: Avoid running query to self through messaging s...
Posted by ifesdjeen <gi...@git.apache.org>.
Github user ifesdjeen commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/278#discussion_r223868092
--- Diff: src/java/org/apache/cassandra/service/reads/repair/AbstractReadRepair.java ---
@@ -102,12 +104,24 @@ void sendReadCommand(Replica to, ReadCallback readCallback, boolean speculative)
else type = to.isFull() ? "full" : "transient";
Tracing.trace("Enqueuing {} data read to {}", type, to);
}
- MessageOut<ReadCommand> message = command.createMessage();
- // if enabled, request additional info about repaired data from any full replicas
- if (command.isTrackingRepairedStatus() && to.isFull())
- message = message.withParameter(ParameterType.TRACK_REPAIRED_DATA, MessagingService.ONE_BYTE);
- MessagingService.instance().sendRRWithFailure(message, to.endpoint(), readCallback);
+ if (to.isSelf())
+ {
+ try (ReadExecutionController executionController = command.executionController();
--- End diff --
Yes, you're absolutely right: it is wrong to block this thread for I/O. We in fact have a pattern in order to deal with these things on local execution path: https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/service/reads/AbstractReadExecutor.java#L157 https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/service/reads/ShortReadPartitionsProtection.java#L186
Thanks for pointing that out!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #278: Avoid running query to self through messaging s...
Posted by ifesdjeen <gi...@git.apache.org>.
Github user ifesdjeen commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/278#discussion_r224060251
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1078,7 +1078,7 @@ public void sendOneWay(MessageOut message, int id, InetAddressAndPort to)
logger.trace("{} sending {} to {}@{}", FBUtilities.getBroadcastAddressAndPort(), message.verb, id, to);
if (to.equals(FBUtilities.getBroadcastAddressAndPort()))
- logger.trace("Message-to-self {} going over MessagingService", message);
+ logger.debug("Message-to-self {} going over MessagingService", message);
--- End diff --
To be honest, I think relying on the logging might have been a bad idea. I've left this statement with `trace` and have rewritten the tests instead.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #278: Avoid running query to self through messaging s...
Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/278#discussion_r223876124
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1078,7 +1078,7 @@ public void sendOneWay(MessageOut message, int id, InetAddressAndPort to)
logger.trace("{} sending {} to {}@{}", FBUtilities.getBroadcastAddressAndPort(), message.verb, id, to);
if (to.equals(FBUtilities.getBroadcastAddressAndPort()))
- logger.trace("Message-to-self {} going over MessagingService", message);
+ logger.debug("Message-to-self {} going over MessagingService", message);
--- End diff --
This really needs to be NoSpamLogger? How many times a second might we do this in some cases? What if new code comes that causes this to occur many times a second?
In OSS debug logging is on all the time. That's really not a risk we should be taking.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org