You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "aweisberg (via GitHub)" <gi...@apache.org> on 2024/01/26 21:59:38 UTC
[PR] Fix timeouts caused by infrequently used Meters [cassandra]
aweisberg opened a new pull request, #3068:
URL: https://github.com/apache/cassandra/pull/3068
(no comment)
--
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
Re: [PR] Fix timeouts caused by infrequently used Meters [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on code in PR #3068:
URL: https://github.com/apache/cassandra/pull/3068#discussion_r1478337120
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -250,6 +271,43 @@ private void removeAlias(MetricName name)
if (mBeanServer.isRegistered(name.getMBeanName()))
MBeanWrapper.instance.unregisterMBean(name.getMBeanName(), MBeanWrapper.OnException.IGNORE);
}
+
+ /**
+ * Very infrequently used meters generate a linear amount of tick work based on how long it has been
+ * since the meter was last marked or read. On scales of a year this can be enough to cause the first request
+ * that needs to mark the meter to time out. Once a day read every meter to force them to run Meter.tickIfNecessary
+ * so we only ever run at most one day worth of tick work per meter in the request path.
+ *
+ * This can be removed if we ever upgrade and switch the default MovingAverage from EWMA to SlidingWindowTimeAverages
+ */
+ private void tickMeters()
+ {
+ List<Throwable> failures = new ArrayList<>();
+ int droppedFailures = 0;
+ for (Meter meter : getMeters().values())
Review Comment:
I would change
```
for (Meter meter : getMeters().values())
{
...
}
```
to the following
```
for (Metric metric : getMetrics().values())
{
if (!Meter.class.isInstance(metric))
continue;
...
}
```
--
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
Re: [PR] Fix timeouts caused by infrequently used Meters [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg closed pull request #3068: Fix timeouts caused by infrequently used Meters
URL: https://github.com/apache/cassandra/pull/3068
--
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
Re: [PR] Fix timeouts caused by infrequently used Meters [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on PR #3068:
URL: https://github.com/apache/cassandra/pull/3068#issuecomment-1924390847
TY for the review! Pushed an update fixing the once a day ticking. It was also making the tests fail so that's a big help.
--
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
Re: [PR] Fix timeouts caused by infrequently used Meters [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on code in PR #3068:
URL: https://github.com/apache/cassandra/pull/3068#discussion_r1478286132
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -250,6 +271,43 @@ private void removeAlias(MetricName name)
if (mBeanServer.isRegistered(name.getMBeanName()))
MBeanWrapper.instance.unregisterMBean(name.getMBeanName(), MBeanWrapper.OnException.IGNORE);
}
+
+ /**
+ * Very infrequently used meters generate a linear amount of tick work based on how long it has been
+ * since the meter was last marked or read. On scales of a year this can be enough to cause the first request
+ * that needs to mark the meter to time out. Once a day read every meter to force them to run Meter.tickIfNecessary
+ * so we only ever run at most one day worth of tick work per meter in the request path.
+ *
+ * This can be removed if we ever upgrade and switch the default MovingAverage from EWMA to SlidingWindowTimeAverages
+ */
+ private void tickMeters()
+ {
+ List<Throwable> failures = new ArrayList<>();
+ int droppedFailures = 0;
+ for (Meter meter : getMeters().values())
Review Comment:
This is actually +2 extra lines of code! Anyway, it's not important for the workaround itself, the comment is just based on the assumption that if we can have zero impact, why shouldn't we? So I agree with you that the node can live without it and this is not a place where we should save resources :-)
--
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
Re: [PR] Fix timeouts caused by infrequently used Meters [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on code in PR #3068:
URL: https://github.com/apache/cassandra/pull/3068#discussion_r1476139392
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -250,6 +271,43 @@ private void removeAlias(MetricName name)
if (mBeanServer.isRegistered(name.getMBeanName()))
MBeanWrapper.instance.unregisterMBean(name.getMBeanName(), MBeanWrapper.OnException.IGNORE);
}
+
+ /**
+ * Very infrequently used meters generate a linear amount of tick work based on how long it has been
+ * since the meter was last marked or read. On scales of a year this can be enough to cause the first request
+ * that needs to mark the meter to time out. Once a day read every meter to force them to run Meter.tickIfNecessary
+ * so we only ever run at most one day worth of tick work per meter in the request path.
+ *
+ * This can be removed if we ever upgrade and switch the default MovingAverage from EWMA to SlidingWindowTimeAverages
+ */
+ private void tickMeters()
+ {
+ List<Throwable> failures = new ArrayList<>();
+ int droppedFailures = 0;
+ for (Meter meter : getMeters().values())
+ {
+ try
+ {
+ meter.getOneMinuteRate();
+ }
+ catch (Throwable t)
+ {
+ if (failures.size() < 10)
+ failures.add(t);
+ else
+ droppedFailures++;
+ }
+ }
+ if (!failures.isEmpty())
+ {
+ Throwable failure = null;
+ for (Throwable t : failures)
+ failure = Throwables.merge(failure, t);
+ // To avoid the scheduled task being cancelled don't leak exceptions
+ // Runs only once a day so noise is not an issue
+ logger.error(String.format("Had error(s) attempting to tick meter. Dropped %d exceptions.", droppedFailures), failure);
Review Comment:
Should we use the `warn` (or `info`) type instead? I don't think a user can fix this error anyway.
--
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
Re: [PR] Fix timeouts caused by infrequently used Meters [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on code in PR #3068:
URL: https://github.com/apache/cassandra/pull/3068#discussion_r1476137342
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -250,6 +271,43 @@ private void removeAlias(MetricName name)
if (mBeanServer.isRegistered(name.getMBeanName()))
MBeanWrapper.instance.unregisterMBean(name.getMBeanName(), MBeanWrapper.OnException.IGNORE);
}
+
+ /**
+ * Very infrequently used meters generate a linear amount of tick work based on how long it has been
+ * since the meter was last marked or read. On scales of a year this can be enough to cause the first request
+ * that needs to mark the meter to time out. Once a day read every meter to force them to run Meter.tickIfNecessary
+ * so we only ever run at most one day worth of tick work per meter in the request path.
+ *
+ * This can be removed if we ever upgrade and switch the default MovingAverage from EWMA to SlidingWindowTimeAverages
+ */
+ private void tickMeters()
+ {
+ List<Throwable> failures = new ArrayList<>();
+ int droppedFailures = 0;
+ for (Meter meter : getMeters().values())
Review Comment:
I think it is more efficient to filter the metrics by type instead of calling the `getMeters()` method, which creates a new map each time it is called - it is expensive for a large set of metrics.
--
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
Re: [PR] Fix timeouts caused by infrequently used Meters [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on code in PR #3068:
URL: https://github.com/apache/cassandra/pull/3068#discussion_r1476124065
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -41,15 +57,20 @@
*/
public class CassandraMetricsRegistry extends MetricRegistry
{
- public static final CassandraMetricsRegistry Metrics = new CassandraMetricsRegistry();
+ private static final Logger logger = LoggerFactory.getLogger(CassandraMetricsRegistry.class);
+
+ public static final CassandraMetricsRegistry Metrics = new CassandraMetricsRegistry(Ints.checkedCast(TimeUnit.MICROSECONDS.toNanos(1)));
Review Comment:
It's noted in the JIRA issue itself that it's enough to read the metrics once a day, but `MICROSECONDS` is here. Is it correct?
--
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
Re: [PR] Fix timeouts caused by infrequently used Meters [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on code in PR #3068:
URL: https://github.com/apache/cassandra/pull/3068#discussion_r1478399933
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -250,6 +271,43 @@ private void removeAlias(MetricName name)
if (mBeanServer.isRegistered(name.getMBeanName()))
MBeanWrapper.instance.unregisterMBean(name.getMBeanName(), MBeanWrapper.OnException.IGNORE);
}
+
+ /**
+ * Very infrequently used meters generate a linear amount of tick work based on how long it has been
+ * since the meter was last marked or read. On scales of a year this can be enough to cause the first request
+ * that needs to mark the meter to time out. Once a day read every meter to force them to run Meter.tickIfNecessary
+ * so we only ever run at most one day worth of tick work per meter in the request path.
+ *
+ * This can be removed if we ever upgrade and switch the default MovingAverage from EWMA to SlidingWindowTimeAverages
+ */
+ private void tickMeters()
+ {
+ List<Throwable> failures = new ArrayList<>();
+ int droppedFailures = 0;
+ for (Meter meter : getMeters().values())
Review Comment:
Ah, I got what you were saying backwards I thought you meant I had the opportunity to remove 2 lines which I would jump at :-)
--
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
Re: [PR] Fix timeouts caused by infrequently used Meters [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on code in PR #3068:
URL: https://github.com/apache/cassandra/pull/3068#discussion_r1476237255
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -250,6 +271,43 @@ private void removeAlias(MetricName name)
if (mBeanServer.isRegistered(name.getMBeanName()))
MBeanWrapper.instance.unregisterMBean(name.getMBeanName(), MBeanWrapper.OnException.IGNORE);
}
+
+ /**
+ * Very infrequently used meters generate a linear amount of tick work based on how long it has been
+ * since the meter was last marked or read. On scales of a year this can be enough to cause the first request
+ * that needs to mark the meter to time out. Once a day read every meter to force them to run Meter.tickIfNecessary
+ * so we only ever run at most one day worth of tick work per meter in the request path.
+ *
+ * This can be removed if we ever upgrade and switch the default MovingAverage from EWMA to SlidingWindowTimeAverages
+ */
+ private void tickMeters()
+ {
+ List<Throwable> failures = new ArrayList<>();
+ int droppedFailures = 0;
+ for (Meter meter : getMeters().values())
Review Comment:
This gets called once a day in a background thread. Assume 100 meters per table and 1000 tables it takes about 26 seconds on my laptop. This is to process a days worth of ticks on every meter as if it were unused.
So maybe `0.00003761574` of daily CPU time on an 8 core server. The time is dominated by actually ticking not iterating the metrics.
I really don't think it's worth the extra lines of code to make iteration faster.
--
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
Re: [PR] Fix timeouts caused by infrequently used Meters [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on code in PR #3068:
URL: https://github.com/apache/cassandra/pull/3068#discussion_r1476132042
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -41,15 +57,20 @@
*/
public class CassandraMetricsRegistry extends MetricRegistry
{
- public static final CassandraMetricsRegistry Metrics = new CassandraMetricsRegistry();
+ private static final Logger logger = LoggerFactory.getLogger(CassandraMetricsRegistry.class);
+
+ public static final CassandraMetricsRegistry Metrics = new CassandraMetricsRegistry(Ints.checkedCast(TimeUnit.MICROSECONDS.toNanos(1)));
private final Map<String, ThreadPoolMetrics> threadPoolMetrics = new ConcurrentHashMap<>();
private final MBeanWrapper mBeanServer = MBeanWrapper.instance;
public final static TimeUnit DEFAULT_TIMER_UNIT = TimeUnit.MICROSECONDS;
- private CassandraMetricsRegistry()
+ final ScheduledFuture<?> periodicMeterTicker;
+
+ CassandraMetricsRegistry(int tickMetersPeriodMicros)
Review Comment:
I think we don't need to pass the argument to the constructor and can use a constant variable instead. We can also add a javadoc to this constant so that the default value can be indexed by search engines once the javadocs are published,
e.g.
`/* Schedule a task to read all the meters once a day so it isn’t done in the request path and we have a more incremental amount to process at a time. Default value {@code 1} day. */ `
--
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
Re: [PR] Fix timeouts caused by infrequently used Meters [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on code in PR #3068:
URL: https://github.com/apache/cassandra/pull/3068#discussion_r1478296345
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -250,6 +271,43 @@ private void removeAlias(MetricName name)
if (mBeanServer.isRegistered(name.getMBeanName()))
MBeanWrapper.instance.unregisterMBean(name.getMBeanName(), MBeanWrapper.OnException.IGNORE);
}
+
+ /**
+ * Very infrequently used meters generate a linear amount of tick work based on how long it has been
+ * since the meter was last marked or read. On scales of a year this can be enough to cause the first request
+ * that needs to mark the meter to time out. Once a day read every meter to force them to run Meter.tickIfNecessary
+ * so we only ever run at most one day worth of tick work per meter in the request path.
+ *
+ * This can be removed if we ever upgrade and switch the default MovingAverage from EWMA to SlidingWindowTimeAverages
+ */
+ private void tickMeters()
+ {
+ List<Throwable> failures = new ArrayList<>();
+ int droppedFailures = 0;
+ for (Meter meter : getMeters().values())
Review Comment:
Which lines are the extra lines? I would remove 2 extra lines if I could?
--
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
Re: [PR] Fix timeouts caused by infrequently used Meters [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on code in PR #3068:
URL: https://github.com/apache/cassandra/pull/3068#discussion_r1476249964
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -250,6 +271,43 @@ private void removeAlias(MetricName name)
if (mBeanServer.isRegistered(name.getMBeanName()))
MBeanWrapper.instance.unregisterMBean(name.getMBeanName(), MBeanWrapper.OnException.IGNORE);
}
+
+ /**
+ * Very infrequently used meters generate a linear amount of tick work based on how long it has been
+ * since the meter was last marked or read. On scales of a year this can be enough to cause the first request
+ * that needs to mark the meter to time out. Once a day read every meter to force them to run Meter.tickIfNecessary
+ * so we only ever run at most one day worth of tick work per meter in the request path.
+ *
+ * This can be removed if we ever upgrade and switch the default MovingAverage from EWMA to SlidingWindowTimeAverages
+ */
+ private void tickMeters()
+ {
+ List<Throwable> failures = new ArrayList<>();
+ int droppedFailures = 0;
+ for (Meter meter : getMeters().values())
+ {
+ try
+ {
+ meter.getOneMinuteRate();
+ }
+ catch (Throwable t)
+ {
+ if (failures.size() < 10)
+ failures.add(t);
+ else
+ droppedFailures++;
+ }
+ }
+ if (!failures.isEmpty())
+ {
+ Throwable failure = null;
+ for (Throwable t : failures)
+ failure = Throwables.merge(failure, t);
+ // To avoid the scheduled task being cancelled don't leak exceptions
+ // Runs only once a day so noise is not an issue
+ logger.error(String.format("Had error(s) attempting to tick meter. Dropped %d exceptions.", droppedFailures), failure);
Review Comment:
It's not something that is ever expected so if it does happen things could be pretty broken not just with this ticking functionality. Since it will only log once a day it seems more appropriate to make it an error so it's more visible and can get fixed.
Warn would be fine, but makes it a little easier for errors to get lost in noise. This is once a day so it wouldn't contribute a lot of noise.
--
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
Re: [PR] Fix timeouts caused by infrequently used Meters [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on code in PR #3068:
URL: https://github.com/apache/cassandra/pull/3068#discussion_r1476357474
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -41,15 +57,20 @@
*/
public class CassandraMetricsRegistry extends MetricRegistry
{
- public static final CassandraMetricsRegistry Metrics = new CassandraMetricsRegistry();
+ private static final Logger logger = LoggerFactory.getLogger(CassandraMetricsRegistry.class);
+
+ public static final CassandraMetricsRegistry Metrics = new CassandraMetricsRegistry(Ints.checkedCast(TimeUnit.MICROSECONDS.toNanos(1)));
private final Map<String, ThreadPoolMetrics> threadPoolMetrics = new ConcurrentHashMap<>();
private final MBeanWrapper mBeanServer = MBeanWrapper.instance;
public final static TimeUnit DEFAULT_TIMER_UNIT = TimeUnit.MICROSECONDS;
- private CassandraMetricsRegistry()
+ final ScheduledFuture<?> periodicMeterTicker;
+
+ CassandraMetricsRegistry(int tickMetersPeriodMicros)
Review Comment:
This is to make it possible for the unit test to create a metrics registry that runs these ticks frequently. To have it be testable there has to be some extension point, either a method that can be overridden or a parameter to the class to allow for manipulating the task scheduling frequency.
--
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
Re: [PR] Fix timeouts caused by infrequently used Meters [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on code in PR #3068:
URL: https://github.com/apache/cassandra/pull/3068#discussion_r1476145864
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -41,15 +57,20 @@
*/
public class CassandraMetricsRegistry extends MetricRegistry
{
- public static final CassandraMetricsRegistry Metrics = new CassandraMetricsRegistry();
+ private static final Logger logger = LoggerFactory.getLogger(CassandraMetricsRegistry.class);
+
+ public static final CassandraMetricsRegistry Metrics = new CassandraMetricsRegistry(Ints.checkedCast(TimeUnit.MICROSECONDS.toNanos(1)));
Review Comment:
Yeah it should be days to micros here.
--
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
Re: [PR] Fix timeouts caused by infrequently used Meters [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on code in PR #3068:
URL: https://github.com/apache/cassandra/pull/3068#discussion_r1476249964
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -250,6 +271,43 @@ private void removeAlias(MetricName name)
if (mBeanServer.isRegistered(name.getMBeanName()))
MBeanWrapper.instance.unregisterMBean(name.getMBeanName(), MBeanWrapper.OnException.IGNORE);
}
+
+ /**
+ * Very infrequently used meters generate a linear amount of tick work based on how long it has been
+ * since the meter was last marked or read. On scales of a year this can be enough to cause the first request
+ * that needs to mark the meter to time out. Once a day read every meter to force them to run Meter.tickIfNecessary
+ * so we only ever run at most one day worth of tick work per meter in the request path.
+ *
+ * This can be removed if we ever upgrade and switch the default MovingAverage from EWMA to SlidingWindowTimeAverages
+ */
+ private void tickMeters()
+ {
+ List<Throwable> failures = new ArrayList<>();
+ int droppedFailures = 0;
+ for (Meter meter : getMeters().values())
+ {
+ try
+ {
+ meter.getOneMinuteRate();
+ }
+ catch (Throwable t)
+ {
+ if (failures.size() < 10)
+ failures.add(t);
+ else
+ droppedFailures++;
+ }
+ }
+ if (!failures.isEmpty())
+ {
+ Throwable failure = null;
+ for (Throwable t : failures)
+ failure = Throwables.merge(failure, t);
+ // To avoid the scheduled task being cancelled don't leak exceptions
+ // Runs only once a day so noise is not an issue
+ logger.error(String.format("Had error(s) attempting to tick meter. Dropped %d exceptions.", droppedFailures), failure);
Review Comment:
It's not something that is ever expected so if it does happen things could be pretty broken not just with this ticking functionality. Since it will only log once a day it seems more appropriate to make it an error so it's more visible and can get fixed.
--
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
Re: [PR] Fix timeouts caused by infrequently used Meters [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on code in PR #3068:
URL: https://github.com/apache/cassandra/pull/3068#discussion_r1476336896
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -250,6 +271,43 @@ private void removeAlias(MetricName name)
if (mBeanServer.isRegistered(name.getMBeanName()))
MBeanWrapper.instance.unregisterMBean(name.getMBeanName(), MBeanWrapper.OnException.IGNORE);
}
+
+ /**
+ * Very infrequently used meters generate a linear amount of tick work based on how long it has been
+ * since the meter was last marked or read. On scales of a year this can be enough to cause the first request
+ * that needs to mark the meter to time out. Once a day read every meter to force them to run Meter.tickIfNecessary
+ * so we only ever run at most one day worth of tick work per meter in the request path.
+ *
+ * This can be removed if we ever upgrade and switch the default MovingAverage from EWMA to SlidingWindowTimeAverages
+ */
+ private void tickMeters()
+ {
+ List<Throwable> failures = new ArrayList<>();
+ int droppedFailures = 0;
+ for (Meter meter : getMeters().values())
+ {
+ try
+ {
+ meter.getOneMinuteRate();
+ }
+ catch (Throwable t)
+ {
+ if (failures.size() < 10)
+ failures.add(t);
+ else
+ droppedFailures++;
+ }
+ }
+ if (!failures.isEmpty())
+ {
+ Throwable failure = null;
+ for (Throwable t : failures)
+ failure = Throwables.merge(failure, t);
+ // To avoid the scheduled task being cancelled don't leak exceptions
+ // Runs only once a day so noise is not an issue
+ logger.error(String.format("Had error(s) attempting to tick meter. Dropped %d exceptions.", droppedFailures), failure);
Review Comment:
I think whatever the exception may be it will be unrelated to Cassandra anyway, so no need for extra visibility. I don't think this nit is critical to the patch, so have it your way :-)
--
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
Re: [PR] Fix timeouts caused by infrequently used Meters [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on code in PR #3068:
URL: https://github.com/apache/cassandra/pull/3068#discussion_r1478261806
##########
test/unit/org/apache/cassandra/metrics/CassandraMetricsRegistryTest.java:
##########
@@ -156,6 +162,32 @@ public void testTimer()
assertEquals(expectedBucketsWithValues, bucketsWithValues);
}
+ @Test
+ public void testTickMeters() throws InterruptedException
+ {
+ CassandraMetricsRegistry cmr = new CassandraMetricsRegistry(Ints.checkedCast(TimeUnit.SECONDS.toMicros(1)));
Review Comment:
```suggestion
CassandraMetricsRegistry cmr = new CassandraMetricsRegistry(TimeUnit.SECONDS.toMicros(1));
```
--
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