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 2020/03/23 15:43:50 UTC
[GitHub] [cassandra] dcapwell opened a new pull request #482:
CASSANDRA-15650 Fix flaky test
org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
dcapwell opened a new pull request #482: CASSANDRA-15650 Fix flaky test org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
URL: https://github.com/apache/cassandra/pull/482
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell closed pull request #482: CASSANDRA-15650 Fix
flaky test org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
Posted by GitBox <gi...@apache.org>.
dcapwell closed pull request #482: CASSANDRA-15650 Fix flaky test org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
URL: https://github.com/apache/cassandra/pull/482
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a change in pull request #482:
CASSANDRA-15650 Fix flaky test
org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #482: CASSANDRA-15650 Fix flaky test org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
URL: https://github.com/apache/cassandra/pull/482#discussion_r399680186
##########
File path: src/java/org/apache/cassandra/repair/RepairRunnable.java
##########
@@ -227,8 +227,14 @@ private void complete(String msg)
public void run()
{
+ String previousName = Thread.currentThread().getName();
try
{
+ Thread.currentThread().setName(NamedThreadFactory.globalPrefix()
+ + "Repair Coordinator #" + cmd
+ + " id=" + parentSession
+ + " ks=" + keyspace
+ + " tables=" + options.getColumnFamilies());
Review comment:
Why do you say that? This is a common practice to make thread dumps easier to reason about.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #482:
CASSANDRA-15650 Fix flaky test
org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #482: CASSANDRA-15650 Fix flaky test org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
URL: https://github.com/apache/cassandra/pull/482#discussion_r399164975
##########
File path: src/java/org/apache/cassandra/tools/RepairRunner.java
##########
@@ -59,19 +64,38 @@ public void run() throws Exception
if (cmd <= 0)
{
// repairAsync can only return 0 for replication factor 1.
- String message = String.format("[%s] Replication factor is 1. No repair is needed for keyspace '%s'", format.format(System.currentTimeMillis()), keyspace);
- out.println(message);
+ String message = String.format("Replication factor is 1. No repair is needed for keyspace '%s'", keyspace);
+ printMessage(message);
}
else
{
- while (!condition.await(NodeProbe.JMX_NOTIFICATION_POLL_INTERVAL_SECONDS, TimeUnit.SECONDS))
+ String previousName = Thread.currentThread().getName();
+ try
{
- queryForCompletedRepair(String.format("After waiting for poll interval of %s seconds",
- NodeProbe.JMX_NOTIFICATION_POLL_INTERVAL_SECONDS));
+ Thread.currentThread().setName("RepairRunner #" + cmd
+ + " ks=" + keyspace
+ + " tables=" + options.get(RepairOption.COLUMNFAMILIES_KEY));
Review comment:
Same comment as before. Thread name should stay constant. Use logging instead.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #482:
CASSANDRA-15650 Fix flaky test
org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #482: CASSANDRA-15650 Fix flaky test org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
URL: https://github.com/apache/cassandra/pull/482#discussion_r399185294
##########
File path: src/java/org/apache/cassandra/utils/Throwables.java
##########
@@ -230,6 +230,24 @@ public static RuntimeException unchecked(Throwable t)
return t instanceof RuntimeException ? (RuntimeException)t : new RuntimeException(t);
}
+ /**
+ * throw the exception without wrapping as a runtime exception. This relies on a trick to stop the compiler from
+ * forcing checking of checked exceptions so a exception can be rethrown unchecked
+ */
+ public static RuntimeException throwAsUncheckedException(Throwable t)
Review comment:
This is based on something that we cannot guaranty will work in the future and I am not truly convinced of its benefit over wrapping the Exception in a Runtime one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #482:
CASSANDRA-15650 Fix flaky test
org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #482: CASSANDRA-15650 Fix flaky test org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
URL: https://github.com/apache/cassandra/pull/482#discussion_r399163058
##########
File path: src/java/org/apache/cassandra/tools/NodeProbe.java
##########
@@ -1807,6 +1805,11 @@ public MessagingServiceMBean getMessagingServiceProxy()
{
return msProxy;
}
+
+ public long getJmxNotificationPollIntervalSeconds()
+ {
+ return Long.getLong("cassandra.nodetool.jmx_notification_poll_interval_seconds", TimeUnit.SECONDS.convert(5, TimeUnit.MINUTES));
+ }
Review comment:
It is not clear to me what is the advantage of using a method over a constant.
Usually all system variables within C* are read and stored within constants.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #482:
CASSANDRA-15650 Fix flaky test
org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #482: CASSANDRA-15650 Fix flaky test org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
URL: https://github.com/apache/cassandra/pull/482#discussion_r400045766
##########
File path: src/java/org/apache/cassandra/repair/RepairRunnable.java
##########
@@ -227,8 +227,14 @@ private void complete(String msg)
public void run()
{
+ String previousName = Thread.currentThread().getName();
try
{
+ Thread.currentThread().setName(NamedThreadFactory.globalPrefix()
+ + "Repair Coordinator #" + cmd
+ + " id=" + parentSession
+ + " ks=" + keyspace
+ + " tables=" + options.getColumnFamilies());
Review comment:
Thread dumps are only one of the tools that use thread names. Logs, JConsole, Profilers also use them. If the name of a thread keep on changing, it is pretty hard to understand what is truly going on and you cannot monitor what happen to a given thread over time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a change in pull request #482:
CASSANDRA-15650 Fix flaky test
org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #482: CASSANDRA-15650 Fix flaky test org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
URL: https://github.com/apache/cassandra/pull/482#discussion_r399680281
##########
File path: src/java/org/apache/cassandra/tools/NodeProbe.java
##########
@@ -1807,6 +1805,11 @@ public MessagingServiceMBean getMessagingServiceProxy()
{
return msProxy;
}
+
+ public long getJmxNotificationPollIntervalSeconds()
+ {
+ return Long.getLong("cassandra.nodetool.jmx_notification_poll_interval_seconds", TimeUnit.SECONDS.convert(5, TimeUnit.MINUTES));
+ }
Review comment:
I went down the path of trying to make different tests have different values but I got rid of that, can revert thus
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a change in pull request #482:
CASSANDRA-15650 Fix flaky test
org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #482: CASSANDRA-15650 Fix flaky test org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
URL: https://github.com/apache/cassandra/pull/482#discussion_r400528643
##########
File path: src/java/org/apache/cassandra/repair/RepairRunnable.java
##########
@@ -227,8 +227,14 @@ private void complete(String msg)
public void run()
{
+ String previousName = Thread.currentThread().getName();
try
{
+ Thread.currentThread().setName(NamedThreadFactory.globalPrefix()
+ + "Repair Coordinator #" + cmd
+ + " id=" + parentSession
+ + " ks=" + keyspace
+ + " tables=" + options.getColumnFamilies());
Review comment:
I disagree that it makes it harder since it provides a lot more context that enhance logs/jconsole. most profilers won't be tricked (at least the ones I use), so reports just have more context in the names; this change is not required for the patch, so I will revert both.
I do plan to add these back though when I start working on the repair observability tickets.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #482:
CASSANDRA-15650 Fix flaky test
org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #482: CASSANDRA-15650 Fix flaky test org.apache.cassandra.distributed.test.*RepairCoordinatorFastTest
URL: https://github.com/apache/cassandra/pull/482#discussion_r399160611
##########
File path: src/java/org/apache/cassandra/repair/RepairRunnable.java
##########
@@ -227,8 +227,14 @@ private void complete(String msg)
public void run()
{
+ String previousName = Thread.currentThread().getName();
try
{
+ Thread.currentThread().setName(NamedThreadFactory.globalPrefix()
+ + "Repair Coordinator #" + cmd
+ + " id=" + parentSession
+ + " ks=" + keyspace
+ + " tables=" + options.getColumnFamilies());
Review comment:
Thread names should always stay the same. Nobody will assume that they might changes and everybody rely on them for debugging.
If additional information is required, it should be logged.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org